Bug 103621

Created attachment 176715 [details] Patch

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.

Created attachment 177703 [details] Patch

(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 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?

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

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

(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()

(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?

Created attachment 181276 [details] Patch

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

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

(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?

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

Created attachment 181689 [details] Patch

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

Created attachment 181843 [details] Patch

(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 on attachment 181843 [details] Patch Clearing flags on attachment: 181843 Committed r139166: <http://trac.webkit.org/changeset/139166>

All reviewed patches have been landed. Closing bug.