Bug 34880 - REGRESSION: SVG text disappears after double click
Summary: REGRESSION: SVG text disappears after double click
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://www.w3.org/Graphics/SVG/Test/2...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-12 00:14 PST by Dirk Schulze
Modified: 2010-02-16 18:49 PST (History)
3 users (show)

See Also:


Attachments
Screenshot of the selecting bug (14.04 KB, image/png)
2010-02-12 00:14 PST, Dirk Schulze
no flags Details
reduction (348 bytes, image/svg+xml)
2010-02-12 06:03 PST, MORITA Hajime
no flags Details
patch v0 (35.77 KB, patch)
2010-02-12 16:44 PST, MORITA Hajime
no flags Details | Formatted Diff | Diff
patch v1 (36.98 KB, patch)
2010-02-16 07:59 PST, MORITA Hajime
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2010-02-12 00:14:58 PST
Created attachment 48628 [details]
Screenshot of the selecting bug

If you double click on a text, it disappears. This doesn't happen on manually selection or pressing Ctrl+A. This is a regression, older revisions don't have this problem. But not all text elements  disappear like "$Revision: 1.6 $" in the W3C-test suite. It's strange, because it's also just a <text>-node like the other texts. The only difference I can see, is that "$Revision: 1.6 $" is not part of a container. It is a direct child of <svg>. But maybe this is accidental.

The last changes were on bug 15997 . Maybe this causes this regression?
Comment 1 MORITA Hajime 2010-02-12 06:03:18 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...
Comment 2 Nikolas Zimmermann 2010-02-12 06:06:10 PST
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.
Comment 3 MORITA Hajime 2010-02-12 16:44:05 PST
Created attachment 48679 [details]
patch v0
Comment 4 MORITA Hajime 2010-02-12 16:48:48 PST
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 5 Nikolas Zimmermann 2010-02-15 17:21:32 PST
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 6 Nikolas Zimmermann 2010-02-15 17:26:37 PST
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?
Comment 7 MORITA Hajime 2010-02-15 22:12:36 PST
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.
Comment 8 MORITA Hajime 2010-02-16 07:59:32 PST
Created attachment 48814 [details]
patch v1
Comment 9 Nikolas Zimmermann 2010-02-16 16:22:29 PST
Comment on attachment 48814 [details]
patch v1

Thanks for the explaination. Looks much better now, r=me.
Comment 10 WebKit Commit Bot 2010-02-16 18:49:42 PST
Comment on attachment 48814 [details]
patch v1

Clearing flags on attachment: 48814

Committed r54858: <http://trac.webkit.org/changeset/54858>
Comment 11 WebKit Commit Bot 2010-02-16 18:49:46 PST
All reviewed patches have been landed.  Closing bug.