Bug 103621 - Caret is incorrectly painted for a contenteditable <div> containing a <br> in vertical writing mode
Summary: Caret is incorrectly painted for a contenteditable <div> containing a <br> in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Arpita Bahuguna
URL:
Keywords:
Depends on:
Blocks: 106452
  Show dependency treegraph
 
Reported: 2012-11-29 02:19 PST by Arpita Bahuguna
Modified: 2013-01-09 06:51 PST (History)
7 users (show)

See Also:


Attachments
Testcase (310 bytes, text/html)
2012-11-29 02:19 PST, Arpita Bahuguna
no flags Details
Patch (7.10 KB, patch)
2012-11-29 06:31 PST, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (7.35 KB, patch)
2012-12-05 02:17 PST, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (7.26 KB, patch)
2013-01-04 00:01 PST, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (8.40 KB, patch)
2013-01-08 07:19 PST, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (8.52 KB, patch)
2013-01-08 22:00 PST, Arpita Bahuguna
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arpita Bahuguna 2012-11-29 02:19:04 PST
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.
Comment 1 Arpita Bahuguna 2012-11-29 06:31:36 PST
Created attachment 176715 [details]
Patch
Comment 2 Ryosuke Niwa 2012-12-03 13:42:58 PST
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.
Comment 3 Arpita Bahuguna 2012-12-05 02:17:59 PST
Created attachment 177703 [details]
Patch
Comment 4 Arpita Bahuguna 2012-12-05 03:36:15 PST
(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.
Comment 5 Robert Hogan 2012-12-11 10:52:33 PST
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?
Comment 6 Ryosuke Niwa 2012-12-17 12:10:07 PST
(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.
Comment 7 Arpita Bahuguna 2012-12-18 06:45:10 PST
(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.
Comment 8 Ryosuke Niwa 2012-12-18 11:03:46 PST
(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()
Comment 9 Ryosuke Niwa 2012-12-18 11:04:28 PST
(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?
Comment 10 Arpita Bahuguna 2013-01-04 00:01:59 PST
Created attachment 181276 [details]
Patch
Comment 11 Arpita Bahuguna 2013-01-04 00:05:02 PST
(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.
Comment 12 Arpita Bahuguna 2013-01-04 00:15:43 PST
(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().
Comment 13 Ryosuke Niwa 2013-01-04 00:27:05 PST
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.
Comment 14 Arpita Bahuguna 2013-01-04 06:50:33 PST
(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?
Comment 15 Ryosuke Niwa 2013-01-04 10:43:13 PST
(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.
Comment 16 Arpita Bahuguna 2013-01-08 07:19:20 PST
Created attachment 181689 [details]
Patch
Comment 17 Arpita Bahuguna 2013-01-08 16:41:57 PST
Hi rniwa, have uploaded another patch with the changes as suggested by you.
Have introduced linesLogicalBoundingBox() and merged the debug statements into the description.
Comment 18 Ryosuke Niwa 2013-01-08 16:45:28 PST
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.
Comment 19 Arpita Bahuguna 2013-01-08 22:00:26 PST
Created attachment 181843 [details]
Patch
Comment 20 Arpita Bahuguna 2013-01-08 22:02:12 PST
(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.
Comment 21 WebKit Review Bot 2013-01-08 22:57:20 PST
Comment on attachment 181843 [details]
Patch

Clearing flags on attachment: 181843

Committed r139166: <http://trac.webkit.org/changeset/139166>
Comment 22 WebKit Review Bot 2013-01-08 22:57:24 PST
All reviewed patches have been landed.  Closing bug.