Bug 62209 – All pointer-events fail if text has visibility="hidden"

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

All pointer-events fail if text has visibility="none". A text should still fire an pointer event on fill, stroke, painted, even it visibility was set to 'hidden'.

The problem is not RenderSVGText, but RenderBlock::nodeAtPoint that is called by RenderSVGText::nodeAtFloatPoint.

I guess this method should return 'true' in:

        // Hit test contents if we don't have columns.
        if (!hasColumns()) {
            if (hitTestContents(request, result, pointInContainer, scrolledOffset.width(), scrolledOffset.height(), hitTestAction)) { ...

but doesn't. hitTestContents(...) returns 'false'. It returns 'true' if text has not visibility="hidden". Needs further checking.

[Dirk Schulze]

s/visibility="none"/visibility="hidden"/ sorry.

[Dirk Schulze]

Created attachment 96237 [details]
Test for pointer-events on hidden text

Test for pointer-events on hidden text. If it works, an alert should appear with the text "event".

[Dirk Schulze]

The problem is in InlineTextBox::nodeAtPoint(). This method checks if visibility is set to visible. If it is not, it returns false. Thats why we fail.

    if (m_truncation != cFullTruncation && visibleToHitTesting() && rect.intersects(result.rectForPoint(pointInContainer))) {
        ...
    }
    return false;

Can't text ever fire a pointer-event if visibility is hidden on HTML+CSS ?
On SVG we differ between visibleFill, visibleStroke, visiblePaint and fill, stroke, paint. Last three events would fire an pointer-event. Need to check this for HTML.

Adding more people who may have some thoughts on pointer-events and visibility.

[Dirk Schulze]

Ahh, SVG only: http://dev.w3.org/csswg/css3-ui/#pointer-events HTML+CSS just have auto, visible and hidden.

[Rob Buis]

Created attachment 97995 [details]
Patch

[WebKit Review Bot]

Attachment 97995 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/rendering/PointerEventsHitRules.cpp:74:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 97995 [details]
Patch

Attachment 97995 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8918025

New failing tests:
svg/custom/pointer-events-text.svg

[WebKit Review Bot]

Created attachment 97999 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 97995 [details]
Patch

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

Great that you're working on this, r- because of:

> Source/WebCore/rendering/InlineTextBox.cpp:362
> +    PointerEventsHitRules::EHitTesting hitTesting = PointerEventsHitRules::HTML_HITTESTING;
> +#if ENABLE(SVG)
> +    if (isSVGInlineTextBox())
> +        hitTesting = PointerEventsHitRules::SVG_TEXT_HITTESTING;
> +#endif
> +    PointerEventsHitRules hitRules(hitTesting, request, renderer()->style()->pointerEvents());

Does HTML really want to use the pointer-events codepath? What for?
Why not override nodeAtPoint in SVGInlineTextBox and handling pointer-events there, just like it's done for all non-text renderers, that support pointer-events.

> LayoutTests/svg/custom/intersection-list-nested-svg.svg:1
> +<?xml version="1.0" encoding="UTF-8"?>

This test looks unrelated.

[Rob Buis]

Hi Niko,

(In reply to comment #9)
> (From update of attachment 97995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97995&action=review
> 
> Great that you're working on this, r- because of:
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:362
> > +    PointerEventsHitRules::EHitTesting hitTesting = PointerEventsHitRules::HTML_HITTESTING;
> > +#if ENABLE(SVG)
> > +    if (isSVGInlineTextBox())
> > +        hitTesting = PointerEventsHitRules::SVG_TEXT_HITTESTING;
> > +#endif
> > +    PointerEventsHitRules hitRules(hitTesting, request, renderer()->style()->pointerEvents());
> 
> Does HTML really want to use the pointer-events codepath? What for?

Yep, see the link Dirk gave in #4.

> Why not override nodeAtPoint in SVGInlineTextBox and handling pointer-events there, just like it's done for all non-text renderers, that support pointer-events.

That is possible, I did consider it. Disadvantage is it duplicated some code AFAICS.
Other options:

- make visibleToHitTesting virtual.
- ifdef SVG section in nodeAtPoint to use PointerEventsHitRules only for SVG InlineTextBox.

Note that if you look at current visibleToHitTesting implementation and the link Dirk provided it is actually wrong ; for pointer-events="all" it should work even if the text is not visible.

I think the solution we choose should be the least intrusive for non SVG InlineTextBox, so indeed providing our own nodeAtPoint may be best(still has code duplication problem).
 
> > LayoutTests/svg/custom/intersection-list-nested-svg.svg:1
> > +<?xml version="1.0" encoding="UTF-8"?>
> 
> This test looks unrelated.

Yes, some svn problem :(
Cheers,

Rob.

[Rob Buis]

Created attachment 98031 [details]
Patch

[Martin Robinson]

Comment on attachment 98031 [details]
Patch

Attachment 98031 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8912983

[WebKit Review Bot]

Comment on attachment 98031 [details]
Patch

Attachment 98031 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8914985

[Rob Buis]

Created attachment 98035 [details]
Patch

[Early Warning System Bot]

Comment on attachment 98031 [details]
Patch

Attachment 98031 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8914989

[WebKit Review Bot]

Comment on attachment 98035 [details]
Patch

Attachment 98035 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8919057

New failing tests:
svg/custom/pointer-events-text.svg

[WebKit Review Bot]

Created attachment 98040 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 98035 [details]
Patch

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

Almost there, sorry needs another iteration:

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:732
> +    if (isLineBreak())

AFAIK this is not possible - we have no <br> in SVG. Turn this into an assertion.

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:736
> +    bool isVisible = (renderer()->style()->visibility() == VISIBLE);

The braces are useless.

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:744
> +                renderer()->updateHitTestResult(result, flipForWritingMode(pointInContainer - toSize(accumulatedOffset)));

Hm, none of the SVG code is HTML-writing-mode-aware. Our vertical text support works differently than HTMLs newly added writing-mode support.
For now, the best is to remove writing mode related flips and tweaks.

Same for the truncation(). We don't support any truncation, so that code can be removed.
Also locationIncludingFlipping() is probably not right, as it flippes based on writing mode as well.

[Rob Buis]

Created attachment 98053 [details]
Patch

[Dirk Schulze]

Comment on attachment 98053 [details]
Patch

r=me Please add a line to the changelog, that an existing test checks the behavior.

[Rob Buis]

Committed r89381: <http://trac.webkit.org/changeset/89381>

[Nikolas Zimmermann]

(In reply to comment #20)
> (From update of attachment 98053 [details])
> r=me Please add a line to the changelog, that an existing test checks the behavior.

Oops, both of you missed:
"+            FloatPoint boxOrigin = locationIncludingFlipping();"

My comment here was "Also locationIncludingFlipping() is probably not right, as it flippes based on writing mode as well.".

Can you please fix this in a follow-up patch?

[Rob Buis]

Hi Niko,

(In reply to comment #22)
> (In reply to comment #20)
> > (From update of attachment 98053 [details] [details])
> > r=me Please add a line to the changelog, that an existing test checks the behavior.
> 
> Oops, both of you missed:
> "+            FloatPoint boxOrigin = locationIncludingFlipping();"
> 
> My comment here was "Also locationIncludingFlipping() is probably not right, as it flippes based on writing mode as well.".
> 
> Can you please fix this in a follow-up patch?

Hmm, so you'd rather have FloatPoint(x(), y())? OTOH going through this code path before my patch worked fine? Also note that rtl, which I think we support (not sure if hit-testing was ever tested on rtl SVG text though), is handled in locationIncludingFlipping. I am not very familiar with this code so hence my question :)
Cheers,

Rob.

[Nikolas Zimmermann]

(In reply to comment #23)
> Hi Niko,
> 
> (In reply to comment #22)
> > (In reply to comment #20)
> > > (From update of attachment 98053 [details] [details] [details])
> > > r=me Please add a line to the changelog, that an existing test checks the behavior.
> > 
> > Oops, both of you missed:
> > "+            FloatPoint boxOrigin = locationIncludingFlipping();"
> > 
> > My comment here was "Also locationIncludingFlipping() is probably not right, as it flippes based on writing mode as well.".
> > 
> > Can you please fix this in a follow-up patch?
> 
> Hmm, so you'd rather have FloatPoint(x(), y())? OTOH going through this code path before my patch worked fine? Also note that rtl, which I think we support (not sure if hit-testing was ever tested on rtl SVG text though), is handled in locationIncludingFlipping. I am not very familiar with this code so hence my question :)
We used the writing mode aware code path for text by accident before, as we didn't provide a nodeAtPoint impl that was pointer-events aware. I'm not yet sure how relevant this is for SVG at all (we handle vertical text differently, but I'm sure writing-mode has a broader scope than just text).
Anyhow, now it's a mixture -- some of the variables you used are writing mode aware, others not, this is inconsistent. I'm not saying we don't want to use the "flipped" code paths at all, just not now :-)

[Rob Buis]

Committed r89710: <http://trac.webkit.org/changeset/89710>