Bug 62209 - All pointer-events fail if text has visibility="hidden"
Summary: All pointer-events fail if text has visibility="hidden"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL: http://dev.w3.org/SVG/profiles/1.1F2/...
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-07 06:39 PDT by Dirk Schulze
Modified: 2011-06-24 15:32 PDT (History)
10 users (show)

See Also:


Attachments
Test for pointer-events on hidden text (402 bytes, image/svg+xml)
2011-06-07 06:42 PDT, Dirk Schulze
no flags Details
Patch (101.01 KB, patch)
2011-06-21 09:12 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.45 MB, application/zip)
2011-06-21 09:36 PDT, WebKit Review Bot
no flags Details
Patch (98.51 KB, patch)
2011-06-21 11:54 PDT, Rob Buis
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (98.57 KB, patch)
2011-06-21 12:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.22 MB, application/zip)
2011-06-21 12:24 PDT, WebKit Review Bot
no flags Details
Patch (98.51 KB, patch)
2011-06-21 13:48 PDT, Rob Buis
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2011-06-07 06:39:55 PDT
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.
Comment 1 Dirk Schulze 2011-06-07 06:40:29 PDT
s/visibility="none"/visibility="hidden"/ sorry.
Comment 2 Dirk Schulze 2011-06-07 06:42:56 PDT
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".
Comment 3 Dirk Schulze 2011-06-07 23:44:05 PDT
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.
Comment 4 Dirk Schulze 2011-06-07 23:52:00 PDT
Ahh, SVG only: http://dev.w3.org/csswg/css3-ui/#pointer-events HTML+CSS just have auto, visible and hidden.
Comment 5 Rob Buis 2011-06-21 09:12:31 PDT
Created attachment 97995 [details]
Patch
Comment 6 WebKit Review Bot 2011-06-21 09:15:12 PDT
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.
Comment 7 WebKit Review Bot 2011-06-21 09:36:22 PDT
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
Comment 8 WebKit Review Bot 2011-06-21 09:36:27 PDT
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
Comment 9 Nikolas Zimmermann 2011-06-21 09:41:24 PDT
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.
Comment 10 Rob Buis 2011-06-21 09:51:50 PDT
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.
Comment 11 Rob Buis 2011-06-21 11:54:06 PDT
Created attachment 98031 [details]
Patch
Comment 12 Martin Robinson 2011-06-21 11:57:44 PDT
Comment on attachment 98031 [details]
Patch

Attachment 98031 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8912983
Comment 13 WebKit Review Bot 2011-06-21 12:01:16 PDT
Comment on attachment 98031 [details]
Patch

Attachment 98031 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8914985
Comment 14 Rob Buis 2011-06-21 12:07:56 PDT
Created attachment 98035 [details]
Patch
Comment 15 Early Warning System Bot 2011-06-21 12:08:03 PDT
Comment on attachment 98031 [details]
Patch

Attachment 98031 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8914989
Comment 16 WebKit Review Bot 2011-06-21 12:24:00 PDT
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
Comment 17 WebKit Review Bot 2011-06-21 12:24:05 PDT
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
Comment 18 Nikolas Zimmermann 2011-06-21 13:04:28 PDT
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.
Comment 19 Rob Buis 2011-06-21 13:48:55 PDT
Created attachment 98053 [details]
Patch
Comment 20 Dirk Schulze 2011-06-21 13:57:50 PDT
Comment on attachment 98053 [details]
Patch

r=me Please add a line to the changelog, that an existing test checks the behavior.
Comment 21 Rob Buis 2011-06-21 14:13:46 PDT
Committed r89381: <http://trac.webkit.org/changeset/89381>
Comment 22 Nikolas Zimmermann 2011-06-21 22:57:49 PDT
(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?
Comment 23 Rob Buis 2011-06-22 12:10:32 PDT
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.
Comment 24 Nikolas Zimmermann 2011-06-24 11:47:15 PDT
(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 :-)
Comment 25 Rob Buis 2011-06-24 15:32:42 PDT
Committed r89710: <http://trac.webkit.org/changeset/89710>