Bug 34880 – REGRESSION: SVG text disappears after double click

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

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?

[MORITA Hajime]

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...

[Nikolas Zimmermann]

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.

[MORITA Hajime]

Created attachment 48679 [details]
patch v0

[MORITA Hajime]

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.

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

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?

[MORITA Hajime]

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.

[MORITA Hajime]

Created attachment 48814 [details]
patch v1

[Nikolas Zimmermann]

Comment on attachment 48814 [details]
patch v1

Thanks for the explaination. Looks much better now, r=me.

[WebKit Commit Bot]

Comment on attachment 48814 [details]
patch v1

Clearing flags on attachment: 48814

Committed r54858: <http://trac.webkit.org/changeset/54858>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.