Created attachment 178991 [details] Reproduced content We cannot click "click here" in the attached content. We should be able to click "click here".
Created attachment 178992 [details] Patch
Comment on attachment 178992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178992&action=review > Source/WebCore/ChangeLog:9 > + InlineBox::nodeAtPoint don't consider vertical writing mode. > + InlineBox::nodeAtPoint shold flip accumulatedOffset like InlineBox::paint(). Could you elaborate more on what was going on? In particular, it's good to clarify why we need to flip values in accumulatedOffset before passing it to hitTest.
Created attachment 179414 [details] Patch
Comment on attachment 179414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179414&action=review I'm making some grammar suggestions. Please don't feel that I'm being overly picky :) > Source/WebCore/ChangeLog:8 > + If parent of inline box change writing mode, flipping offset is responsibility of inline box. "If the parent of an inline box changes writing mode, then the inline box must flip incoming hit point coordinates." > Source/WebCore/ChangeLog:9 > + If they don't flip offset, they misunderstand offset because their origin is different from their parent's. "Otherwise, the coordinate will be misunderstood because the box has a different origin from its parent." > Source/WebCore/ChangeLog:10 > + nodeAtPoint() in InlineTextBox and InlineFlowBox flip. I don't think you need this line. > Source/WebCore/ChangeLog:12 > + If InlineBox don't flip its offset, children of InlineBox use wrong offset > + because they don't(shouldn't) know that its grand parent changing writing mode. "If the InlineBox doesn't flip its offset, its children will use the wrong offset because they don't know their grandparent changed writing modes." > LayoutTests/ChangeLog:8 > + test that we can click 2nd line inside inline-block in vertical writing mode. Nit: Test (with uppercase T).
Created attachment 179683 [details] Patch
(In reply to comment #4) > (From update of attachment 179414 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179414&action=review > > I'm making some grammar suggestions. Please don't feel that I'm being overly picky :) I'm sorry to make you review grammar, but thank you. It is a big help for me as English learner :) > > Source/WebCore/ChangeLog:8 > > + If parent of inline box change writing mode, flipping offset is responsibility of inline box. > > "If the parent of an inline box changes writing mode, then the inline box must flip incoming hit point coordinates." > > > Source/WebCore/ChangeLog:9 > > + If they don't flip offset, they misunderstand offset because their origin is different from their parent's. > > "Otherwise, the coordinate will be misunderstood because the box has a different origin from its parent." > > > Source/WebCore/ChangeLog:10 > > + nodeAtPoint() in InlineTextBox and InlineFlowBox flip. > > I don't think you need this line. > > > Source/WebCore/ChangeLog:12 > > + If InlineBox don't flip its offset, children of InlineBox use wrong offset > > + because they don't(shouldn't) know that its grand parent changing writing mode. > > "If the InlineBox doesn't flip its offset, its children will use the wrong offset > because they don't know their grandparent changed writing modes." > > > LayoutTests/ChangeLog:8 > > + test that we can click 2nd line inside inline-block in vertical writing mode. > > Nit: Test (with uppercase T). Applied all suggestions.
Comment on attachment 179683 [details] Patch Thanks!
Comment on attachment 179683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179683&action=review > LayoutTests/fast/writing-mode/vertical-inline-block-hittest-expected.txt:2 > + > +b This test can benefit from some brief description on what it’s testing.
Comment on attachment 179683 [details] Patch Clearing flags on attachment: 179683 Committed r138080: <http://trac.webkit.org/changeset/138080>
All reviewed patches have been landed. Closing bug.
Reopening to attach new patch.
Created attachment 180273 [details] Patch
Hi Ryosuke (In reply to comment #8) > (From update of attachment 179683 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179683&action=review > > > LayoutTests/fast/writing-mode/vertical-inline-block-hittest-expected.txt:2 > > + > > +b > > This test can benefit from some brief description on what it’s testing. I added the description. I don't know the rule for this case. I tentatively request review. Please correct me, if I’m wrong.
Comment on attachment 180273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180273&action=review > LayoutTests/fast/writing-mode/vertical-inline-block-hittest.html:19 > + if (hitResult) { > + result.innerHTML = 'PASS: We can click "a" element at 2nd line inside inline-block.'; > + } else { > + result.innerHTML = 'FAIL: We cannot hit "a" element at 2nd line inside inline-block.'; > + } No curly brackets around single statements. It’ll be also nice to hide the div so that “b” won’t get dumped. Also you could make a use of js-test-pre.js and just do: var elementInsideInlineBlockInSecondLine = document.querySelector(‘a’); shouldBe(“document.elementFromPoint(5, 5)”, "elementInsideInlineBlockInSecondLine");
Created attachment 180279 [details] Rewrite test using js-test-pre.js
Has this been landed? Or do we need to land it still?
(In reply to comment #16) > Has this been landed? Or do we need to land it still? Sorry. This patch have not been landed yet. I worry that win EWS fail to patch files, because the failing files are not changed after they have been created. Is this linefeed code problem?
(In reply to comment #17) > (In reply to comment #16) > > Has this been landed? Or do we need to land it still? > > Sorry. This patch have not been landed yet. > > I worry that win EWS fail to patch files, because the failing files are not changed after they have been created. > Is this linefeed code problem? I think Windows bot was just sick. I think you should just land it.
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > Has this been landed? Or do we need to land it still? > > > > Sorry. This patch have not been landed yet. > > > > I worry that win EWS fail to patch files, because the failing files are not changed after they have been created. > > Is this linefeed code problem? > > I think Windows bot was just sick. I think you should just land it. Thank you. I added commit‑queue? flag. Could you land this patch?
Comment on attachment 180279 [details] Rewrite test using js-test-pre.js Clearing flags on attachment: 180279 Committed r140753: <http://trac.webkit.org/changeset/140753>
Thanks a lot!