Summary: | REGRESSION: SVG text disappears after double click | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, morrita, zimmermann | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
URL: | http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-pservers-pattern-01-b.html | ||||||||||||
Attachments: |
|
Description
Dirk Schulze
2010-02-12 00:14:58 PST
Created attachment 48639 [details]
reduction
Thank you for let me know this regression.
I made reduction. It seems to happen when there is at least 2 <text> element.
And it seems to relate repaint algorithm. I'll investigate further...
I think I'm aware of this bug, it's related to RenderSVGText forget to apply the x/y translation offset to the repaint rects - related to a foreignObject bug that I'm just working on. Stay tuned for more information. Created attachment 48679 [details]
patch v0
As krit guessed, the regression was introduced at Bug 15997. To reduce the count of walking subphase, I tried to skip selection related subphase when there is no selection and implement SVGRootInlineBoxPaintWalker::mayHaveSelection(). But its iplementation was wrong. This patch tries to fix it. Comment on attachment 48679 [details]
patch v0
Looks good to me, r=me.
There's a style issue the bot didn't catch:
369 return (box->renderer()->view()->selectionStart() || box->renderer()->view()->selectionEnd());
I'm going to fix that before landing.
Comment on attachment 48679 [details]
patch v0
Oops, I misread the patch and take back review. Checking the RenderView for selection is not sufficient.
Think of a compound document where you're selecting XHTML test, but not anything in the SVG fragment.
Can you explain a bit more detailed what's the problem with selectionStartEnd here?
Hi, Thank you for reviewing! (In reply to comment #6) > Think of a compound document where you're selecting XHTML test, but not > anything in the SVG fragment. > > Can you explain a bit more detailed what's the problem with selectionStartEnd > here? Return value of selectionStartEnd() is a offset from the beginning of the "line", not of the "document". So there are cases where selectionStartPos == selectionEndPos (or selectionStartPos > selectonEndPos) even if we have a selection. For example, when selecting whole single line by triple-clicking a text, selectiondStartPos is located at the beginning of certain line, and selectionEndPos is located at the beginning of the next line. Both selectioniStartPos and selectionEndPos are zero in this case. Because old mayHaveSelection() return false in that case, selection painting got skipped, and finally text of the line disappeared. BTW, skipping subphases with mayHaveSelection() is only for speed, so it is safe even if mayHaveSelection() always returns true. And current implementation is very conservative: it gets false only if there is no selection in the document. And yes, there may be better way to impelemnt mayHaveSelection() using SVGInlineTextBox::selectionStartEnd(). I'll investigate it and re-submit a patch. Created attachment 48814 [details]
patch v1
Comment on attachment 48814 [details]
patch v1
Thanks for the explaination. Looks much better now, r=me.
Comment on attachment 48814 [details] patch v1 Clearing flags on attachment: 48814 Committed r54858: <http://trac.webkit.org/changeset/54858> All reviewed patches have been landed. Closing bug. |