Bug 103621 – Caret is incorrectly painted for a contenteditable <div> containing a <br> in vertical writing mode

• Component: HTML Editing
• Status: RESOLVED FIXED

[Arpita Bahuguna]

Created attachment 176681 [details]
Testcase

The position of the caret is not computed correctly when a contenteditable <div> contains only a <br> element in the vertical writing mode.

If the <br> element is followed by some some text, the caret is drawn correctly, i.e. at the start of the contenteditable block.
Again, in the horizontal writing mode as well the caret is painted at the right location.

Have uploaded a testcase for this issue. A caret should be drawn within the block upon loading the page.

[Arpita Bahuguna]

Created attachment 176715 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 176715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176715&action=review

> Source/WebCore/dom/Position.cpp:845
> +            if ((o->isText() && (o->style()->isHorizontalWritingMode() ? toRenderText(o)->linesBoundingBox().height() : toRenderText(o)->linesBoundingBox().width()))
> +                || (o->isBox() && (o->style()->isHorizontalWritingMode() ? toRenderBox(o)->borderBoundingBox().height() : toRenderBox(o)->borderBoundingBox().width())))

We should be calling logicalWidth & logicalHeight instead.

> LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode.html:17
> +    /* Caret rect in div containing text. */

This comment repeats the code. Please remove it.

> LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode.html:22
> +    /* Caret rect in div containing no text. */

Ditto.

[Arpita Bahuguna]

Created attachment 177703 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #2)
> (From update of attachment 176715 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176715&action=review
> 
Thanks for the review rniwa.

> > Source/WebCore/dom/Position.cpp:845
> > +            if ((o->isText() && (o->style()->isHorizontalWritingMode() ? toRenderText(o)->linesBoundingBox().height() : toRenderText(o)->linesBoundingBox().width()))
> > +                || (o->isBox() && (o->style()->isHorizontalWritingMode() ? toRenderBox(o)->borderBoundingBox().height() : toRenderBox(o)->borderBoundingBox().width())))
> 
> We should be calling logicalWidth & logicalHeight instead.
> 
The width and the height values returned for linesBoundingBox would already be adjusted as per the writing mode, but the same is not true for borderBoundingBox.

Thus we should use logicalWidth and logicalHeight for borderBoundingBox since these return the width and height values in accordance with the writing mode.

As the borderBoundingBox internally calls on the pixelSnappedBorderBoxRect() have made use of the pixelSnappedLogicalHeight() and pixelSnappedLogicalWidth() methods which too take care of the writing mode.

> > LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode.html:17
> > +    /* Caret rect in div containing text. */
> 
> This comment repeats the code. Please remove it.
> 
> > LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode.html:22
> > +    /* Caret rect in div containing no text. */
> 
> Ditto.

Done.

[Robert Hogan]

Comment on attachment 177703 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177703&action=review

> Source/WebCore/dom/Position.cpp:845
> +            if ((o->isText() && (o->style()->isHorizontalWritingMode() ? toRenderText(o)->linesBoundingBox().height() : toRenderText(o)->linesBoundingBox().width()))
> +                || (o->isBox() && (o->style()->isHorizontalWritingMode() ? toRenderBox(o)->pixelSnappedLogicalHeight() : toRenderBox(o)->pixelSnappedLogicalWidth())))

Anyone know why this function doesn't already check the linesBoundingBox of RenderInlines?

[Ryosuke Niwa]

(In reply to comment #4)
> The width and the height values returned for linesBoundingBox would already be adjusted as per the writing mode, but the same is not true for borderBoundingBox.
> 
> Thus we should use logicalWidth and logicalHeight for borderBoundingBox since these return the width and height values in accordance with the writing mode.
> 
> As the borderBoundingBox internally calls on the pixelSnappedBorderBoxRect() have made use of the pixelSnappedLogicalHeight() and pixelSnappedLogicalWidth() methods which too take care of the writing mode.

I don’t get this… pixelSnappedLogicalHeight already checks style()->isHorizontalWritingMode(). If we’re to flip values again, then we’re back to square one. RenderText::linesBoundingBox also takes the writing mode into account.

[Arpita Bahuguna]

(In reply to comment #6)
> (In reply to comment #4)
> > The width and the height values returned for linesBoundingBox would already be adjusted as per the writing mode, but the same is not true for borderBoundingBox.
> > 
> > Thus we should use logicalWidth and logicalHeight for borderBoundingBox since these return the width and height values in accordance with the writing mode.
> > 
> > As the borderBoundingBox internally calls on the pixelSnappedBorderBoxRect() have made use of the pixelSnappedLogicalHeight() and pixelSnappedLogicalWidth() methods which too take care of the writing mode.
> 
> I don’t get this… pixelSnappedLogicalHeight already checks style()->isHorizontalWritingMode(). If we’re to flip values again, then we’re back to square one. RenderText::linesBoundingBox also takes the writing mode into account.

This happens because internally RenderText::linesBoundingBox() does a logicalRightSide - logicalLeftSide to return the height value, when in the vertical writing mode.

Now, our logicalRightSide is nothing but curr->logicalRight() which again internally handles the writing mode.
So in effect we are reversing the value in RenderText::linesBoundingBox() itself and hence the need to reverse it again in Position::hasRenderedNonAnonymousDescendantsWithHeight() arises.

In my opinion our RenderText::linesBoundingBox() should be internally changed to compute values as:
float x = logicalLeftSide;
float y = firstTextBox()->logicalTop();
float width = logicalRightSide - logicalLeftSide;
and
float height = lastTextBox()->logicalBottom() - y;

These take care of the writing mode and also, with this, there is no need for code change in Position::hasRenderedNonAnonymousDescendantsWithHeight().

I propose to put another patch with the fix in RenderText::linesBoundingBox() (as suggested above) instead of the current patch, if it seems correct.

[Ryosuke Niwa]

(In reply to comment #7)
> (In reply to comment #6)
> >
> > I don’t get this… pixelSnappedLogicalHeight already checks style()->isHorizontalWritingMode(). If we’re to flip values again, then we’re back to square one. RenderText::linesBoundingBox also takes the writing mode into account.
> 
> This happens because internally RenderText::linesBoundingBox() does a logicalRightSide - logicalLeftSide to return the height value, when in the vertical writing mode.
> 
> Now, our logicalRightSide is nothing but curr->logicalRight() which again internally handles the writing mode.
> So in effect we are reversing the value in RenderText::linesBoundingBox() itself and hence the need to reverse it again in Position::hasRenderedNonAnonymousDescendantsWithHeight() arises.

Oh, sure. linesBoundingBox case makes sense. What I'm complaining about is:
o->style()->isHorizontalWritingMode() ? toRenderBox(o)->pixelSnappedLogicalHeight() : toRenderBox(o)->pixelSnappedLogicalWidth()

[Ryosuke Niwa]

(In reply to comment #7)
> In my opinion our RenderText::linesBoundingBox() should be internally changed to compute values as:
> float x = logicalLeftSide;
> float y = firstTextBox()->logicalTop();
> float width = logicalRightSide - logicalLeftSide;
> and
> float height = lastTextBox()->logicalBottom() - y;
> 
> These take care of the writing mode and also, with this, there is no need for code change in Position::hasRenderedNonAnonymousDescendantsWithHeight().
> 
> I propose to put another patch with the fix in RenderText::linesBoundingBox() (as suggested above) instead of the current patch, if it seems correct.

That sounds like a sensible thing to do. Perhaps we need to do that in a separate patch? Does all tests pass when you make that change?

[Arpita Bahuguna]

Created attachment 181276 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > >
> > > I don’t get this… pixelSnappedLogicalHeight already checks style()->isHorizontalWritingMode(). If we’re to flip values again, then we’re back to square one. RenderText::linesBoundingBox also takes the writing mode into account.
> > 
> > This happens because internally RenderText::linesBoundingBox() does a logicalRightSide - logicalLeftSide to return the height value, when in the vertical writing mode.
> > 
> > Now, our logicalRightSide is nothing but curr->logicalRight() which again internally handles the writing mode.
> > So in effect we are reversing the value in RenderText::linesBoundingBox() itself and hence the need to reverse it again in Position::hasRenderedNonAnonymousDescendantsWithHeight() arises.
> 
> Oh, sure. linesBoundingBox case makes sense. What I'm complaining about is:
> o->style()->isHorizontalWritingMode() ? toRenderBox(o)->pixelSnappedLogicalHeight() : toRenderBox(o)->pixelSnappedLogicalWidth()

Yes, you are indeed correct and apologize for overseeing this in my previous patch.
For RenderBox we don't really need to have a specific writing mode check again since pixelSnappedLogicalHeight() does take care of returning the appropriate value based on the writing mode.
Have made the required changes in the latest patch.

[Arpita Bahuguna]

(In reply to comment #9)
> (In reply to comment #7)
> > In my opinion our RenderText::linesBoundingBox() should be internally changed to compute values as:
> > float x = logicalLeftSide;
> > float y = firstTextBox()->logicalTop();
> > float width = logicalRightSide - logicalLeftSide;
> > and
> > float height = lastTextBox()->logicalBottom() - y;
> > 
> > These take care of the writing mode and also, with this, there is no need for code change in Position::hasRenderedNonAnonymousDescendantsWithHeight().
> > 
> > I propose to put another patch with the fix in RenderText::linesBoundingBox() (as suggested above) instead of the current patch, if it seems correct.
> 
> That sounds like a sensible thing to do. Perhaps we need to do that in a separate patch? Does all tests pass when you make that change?

I did raise another bug for this issue, based on my prev interpretation in comment #7 above. https://bugs.webkit.org/show_bug.cgi?id=105409

But on further analysis (https://bugs.webkit.org/show_bug.cgi?id=105409#c1) have found the current implementation for RenderText::linesBoundingBox() to be correct.

Even though logicalRight() and logicalLeft() return values based on the writing mode, InlineBox's logicalHeight() and logicalWidth() don't. Their logicalness perhaps refers to the bidi logicalness as you had mentioned earlier.
Thus for our linesBoundingBox we would still require a specific writing mode check in Position::hasRenderedNonAnonymousDescendantsWithHeight().

[Ryosuke Niwa]

Comment on attachment 181276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181276&action=review

> Source/WebCore/ChangeLog:20
> +        case the <br> element) rather than the height in the vertical mode.

Thanks for the detailed change log. It makes the code review much easier.

> Source/WebCore/dom/Position.cpp:849
> +            if ((o->isText() && (o->style()->isHorizontalWritingMode() ? toRenderText(o)->linesBoundingBox().height() : toRenderText(o)->linesBoundingBox().width()))

Can we add something like linesLogicalBoundingBox and flip width & height there?

> LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode-expected.txt:12
> +The caret on the empty div, containing only the <br> element, should be visible. For the test to pass, the top, the width and the height of the caret rects in both the boxes should be the same.
> +PASS withTextCaretRect.top is withoutTextCaretRect.top
> +PASS withTextCaretRect.width is withoutTextCaretRect.width
> +PASS withTextCaretRect.height is withoutTextCaretRect.height
> +To manually verify the issue, try focusing or clicking on the second box. A caret should be visible at the start position of that container.

It's really distracting to see these debug messages interleaving actual test results.

> LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode.html:25
> +    debug("The caret on the empty div, containing only the &lt;br&gt; element, should be visible. For the test to pass, the top, the width and the height of the caret rects in both the boxes should be the same.")

Why don't you merge this debug,

> LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode.html:33
> +    debug("To manually verify the issue, try focusing or clicking on the second box. A caret should be visible at the start position of that container.");

and this one into the description? (Add \n as needed). Also, these lines are really long for my taste.

[Arpita Bahuguna]

(In reply to comment #13)
> (From update of attachment 181276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181276&action=review
> 
Thanks for the review rniwa. I will shortly add another patch with the changes as suggested by you.

> > Source/WebCore/dom/Position.cpp:849
> > +            if ((o->isText() && (o->style()->isHorizontalWritingMode() ? toRenderText(o)->linesBoundingBox().height() : toRenderText(o)->linesBoundingBox().width()))
> 
> Can we add something like linesLogicalBoundingBox and flip width & height there?
> 
This seems like a good idea, would you rather have me do this as a separate fix or as a part of this patch itself?

[Ryosuke Niwa]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 181276 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181276&action=review
> > 
> Thanks for the review rniwa. I will shortly add another patch with the changes as suggested by you.
> 
> > > Source/WebCore/dom/Position.cpp:849
> > > +            if ((o->isText() && (o->style()->isHorizontalWritingMode() ? toRenderText(o)->linesBoundingBox().height() : toRenderText(o)->linesBoundingBox().width()))
> > 
> > Can we add something like linesLogicalBoundingBox and flip width & height there?
> > 
> This seems like a good idea, would you rather have me do this as a separate fix or as a part of this patch itself?

We should do it in this patch.

[Arpita Bahuguna]

Created attachment 181689 [details]
Patch

[Arpita Bahuguna]

Hi rniwa, have uploaded another patch with the changes as suggested by you.
Have introduced linesLogicalBoundingBox() and merged the debug statements into the description.

[Ryosuke Niwa]

Comment on attachment 181689 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181689&action=review

> Source/WebCore/rendering/RenderText.cpp:1617
> +    IntRect rect;
> +
> +    rect = linesBoundingBox();

You can just do:
IntRect rect = linesBoundingBox();

> LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode-expected.txt:3
> +
> +some text
> +

Could you hide this test when ran inside a DRT?

> LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode.html:24
> +        

Nit: whitespace.

> LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode.html:29
> +    }

Setting style.display to none would work.

> LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode.html:30
> +    

Nit: whitespace.

[Arpita Bahuguna]

Created attachment 181843 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #18)
> (From update of attachment 181689 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181689&action=review
> 
Many thanks for the review rniwa. Have uploaded another patch with the suggested changes.

> > Source/WebCore/rendering/RenderText.cpp:1617
> > +    IntRect rect;
> > +
> > +    rect = linesBoundingBox();
> 
> You can just do:
> IntRect rect = linesBoundingBox();
> 
> > LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode-expected.txt:3
> > +
> > +some text
> > +
> 
> Could you hide this test when ran inside a DRT?
> 
> > LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode.html:24
> > +        
> 
> Nit: whitespace.
> 
> > LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode.html:29
> > +    }
> 
> Setting style.display to none would work.
> 
> > LayoutTests/editing/selection/caret-in-div-containing-br-in-vertical-mode.html:30
> > +    
> 
> Nit: whitespace.

[WebKit Review Bot]

Comment on attachment 181843 [details]
Patch

Clearing flags on attachment: 181843

Committed r139166: <http://trac.webkit.org/changeset/139166>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.