Bug 104775 – Cannot click an element at 2nd line or more inside inline-block in vertical writing mode.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Yuki Sekiguchi]

Created attachment 178991 [details]
Reproduced content

We cannot click "click here" in the attached content.

We should be able to click "click here".

[Yuki Sekiguchi]

Created attachment 178992 [details]
Patch

[Ryosuke Niwa]

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.

[Yuki Sekiguchi]

Created attachment 179414 [details]
Patch

[Dean Jackson]

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

[Yuki Sekiguchi]

Created attachment 179683 [details]
Patch

[Yuki Sekiguchi]

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

[Dean Jackson]

Comment on attachment 179683 [details]
Patch

Thanks!

[Ryosuke Niwa]

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.

[WebKit Review Bot]

Comment on attachment 179683 [details]
Patch

Clearing flags on attachment: 179683

Committed r138080: <http://trac.webkit.org/changeset/138080>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuki Sekiguchi]

Reopening to attach new patch.

[Yuki Sekiguchi]

Created attachment 180273 [details]
Patch

[Yuki Sekiguchi]

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.

[Ryosuke Niwa]

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");

[Yuki Sekiguchi]

Created attachment 180279 [details]
Rewrite test using js-test-pre.js

[Ryosuke Niwa]

Has this been landed? Or do we need to land it still?

[Yuki Sekiguchi]

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

[Ryosuke Niwa]

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

[Yuki Sekiguchi]

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

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuki Sekiguchi]

Thanks a lot!