Fix RenderBlock::nodeAtPoint() for scrollbars of hidden and pointer-events:none elements

Fix RenderBlock::nodeAtPoint() for scrollbars of hidden and pointer-events:none elements

Make sure that the RenderBlock is visible to the hit test request before
testing whether the test point is in an overflow control.

This is similar to the code a few lines down that performs hit testing
for the background.

BUG=313908
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164436

[tkent, 2013-11-18 01:21:49]

I'm not familiar with this code.  Adding people who might know.

[leviw_travelin_and_unemployed, 2013-11-18 04:10:33]

On 2013/11/18 01:21:49, tkent wrote:
> I'm not familiar with this code.  Adding people who might know.

I'm unsure of how overflow controls are supposed to be handled with pointer
events. Should they be ignored with pointer-events: none? This patch would have
that effect.

[dtrebbien, 2013-11-18 12:59:33]

On 2013/11/18 04:10:33, Levi wrote:
> I'm unsure of how overflow controls are supposed to be handled with pointer
> events. Should they be ignored with pointer-events: none? This patch would
have
> that effect.

According to the CSS4 UI WG notes at
http://wiki.csswg.org/spec/css4-ui#pointer-events , it seems that
http://www.w3.org/wiki/User:Tantekelik/pointer-events is the latest working
draft of the wording for the pointer-events spec.  Unfortunately, it does not
mention the effect of pointer-events:none on overflow controls, nor does the
issues list for CSS3 UI http://wiki.csswg.org/spec/css3-ui#css3-ui-issues-list

I tested the following in Firefox 25.0.1 and IE 11:
<div id="div1" style="position: absolute; top: 0; left: 0; width: 100px; height:
340px; background-color: Pink; overflow: auto;" onclick="alert('div1 clicked')">
<div id="div1Content" style="width: 100%; height: 500px;">
    Div 1
</div>
</div>
<div id="div2" style="position: absolute; pointer-events: none; top: 20px; left:
0; width: 100px; height: 300px; background-color: Gold; overflow: auto; z-index:
1;" onclick="alert('div2 clicked')">
<div id="div2Content" style="width: 200%; height: 500px;">
    Div 2
</div>
</div>

In Firefox 25.0.1, overflow scrollbars on div2 *do* respond to mouse events, but
in IE 11, overflow scrollbars on div2 do *not* respond to mouse events.  This
patch copies IE 11's behavior in this case.

In both browsers with the mouse cursor over the Gold-colored div2Content area,
scrolling with the mouse wheel scrolls div1.

Personally I think that IE 11's behavior makes better sense here because it's
consistent that pointer-events:none on div2 prevents it from being scrolled.  In
Firefox, if using overflow scrollbars, then it is possible to scroll div2. 
However, if using overlay scrollbars, (e.g. Mac's "Show scroll bars" setting is
changed to "When scrolling") then it is not possible to scroll div2.

[tony, 2013-11-18 17:26:55]

Thanks for looking into this!  Please send an email to www-style asking for
clarifications with the spec text:
http://lists.w3.org/Archives/Public/www-style/

Since IE and Firefox have different behaviors, it would be good to clarify the
spec text.

https://codereview.chromium.org/68243009/diff/50001/LayoutTests/scrollbars/sc...
File LayoutTests/scrollbars/scrollbar-visibility-hidden.html (right):

https://codereview.chromium.org/68243009/diff/50001/LayoutTests/scrollbars/sc...
LayoutTests/scrollbars/scrollbar-visibility-hidden.html:29: var elem =
document.elementFromPoint(95, 120 + 50);
We normally use dom position values (offsetTop/offsetLeft,
offsetWidth/offsetHeight) rather than hard coding values.  Look at a bunch of
the other tests for an example of this.

https://codereview.chromium.org/68243009/diff/50001/LayoutTests/scrollbars/sc...
LayoutTests/scrollbars/scrollbar-visibility-hidden.html:37: // (95, 170) would
be within div2's vertical scrollbar if div2 were still visible.
It would probably be safer to subtract 5px from the right edge of the div. 
Also, I can't remember what we do on OSX.  Do we need to turn on non-overlay
scrollbars or is that the default?

[dtrebbien, 2013-11-19 13:15:32]

On 2013/11/18 17:26:55, tony wrote:
> Thanks for looking into this!  Please send an email to www-style asking for
> clarifications with the spec text:
> http://lists.w3.org/Archives/Public/www-style/
> 
> Since IE and Firefox have different behaviors, it would be good to clarify the
> spec text.

Posted:  http://lists.w3.org/Archives/Public/www-style/2013Nov/0330.html


>
https://codereview.chromium.org/68243009/diff/50001/LayoutTests/scrollbars/sc...
> File LayoutTests/scrollbars/scrollbar-visibility-hidden.html (right):

What does "scrollbar-visibility-hidden.html (right):" mean?


>
https://codereview.chromium.org/68243009/diff/50001/LayoutTests/scrollbars/sc...
> LayoutTests/scrollbars/scrollbar-visibility-hidden.html:29: var elem =
> document.elementFromPoint(95, 120 + 50);
> We normally use dom position values (offsetTop/offsetLeft,
> offsetWidth/offsetHeight) rather than hard coding values.  Look at a bunch of
> the other tests for an example of this.
> 
>
https://codereview.chromium.org/68243009/diff/50001/LayoutTests/scrollbars/sc...
> LayoutTests/scrollbars/scrollbar-visibility-hidden.html:37: // (95, 170) would
> be within div2's vertical scrollbar if div2 were still visible.
> It would probably be safer to subtract 5px from the right edge of the div.

I am working on a revised patch that incorporates these changes, which I hope to
complete and upload either later today or tomorrow morning.


> Also, I can't remember what we do on OSX.  Do we need to turn on non-overlay
> scrollbars or is that the default?

It seems that during testing, overflow scrollbars are turned on.  When I re-ran
the test on Mac with "Show scroll bars" set to "When scrolling", the test
passed, even though it fails when run standalone.

[dtrebbien, 2013-11-19 13:25:23]

On 2013/11/19 13:15:32, dtrebbien wrote:
>
https://codereview.chromium.org/68243009/diff/50001/LayoutTests/scrollbars/sc...
> > File LayoutTests/scrollbars/scrollbar-visibility-hidden.html (right):
> 
> What does "scrollbar-visibility-hidden.html (right):" mean?

Oh, never mind.  I guess it means "referring to the right-hand side of the
diff", which is the only side for this file because it is added.

[dtrebbien, 2013-11-20 12:20:08]

On 2013/11/18 17:26:55, tony wrote:
>
https://codereview.chromium.org/68243009/diff/50001/LayoutTests/scrollbars/sc...
> LayoutTests/scrollbars/scrollbar-visibility-hidden.html:29: var elem =
> document.elementFromPoint(95, 120 + 50);
> We normally use dom position values (offsetTop/offsetLeft,
> offsetWidth/offsetHeight) rather than hard coding values.  Look at a bunch of
> the other tests for an example of this.
> 
>
https://codereview.chromium.org/68243009/diff/50001/LayoutTests/scrollbars/sc...
> LayoutTests/scrollbars/scrollbar-visibility-hidden.html:37: // (95, 170) would
> be within div2's vertical scrollbar if div2 were still visible.
> It would probably be safer to subtract 5px from the right edge of the div. 

I uploaded Patch Set 4 to address your feedback.

[tony, 2013-11-20 17:29:50]

For those following at home, on www-style, Tab said that matching IE11 (which
this patch does) makes sense.

This change looks correct to me, but someone more actively working on blink
probably ought to weigh in.

I kicked off some try jobs.

dtrebblen, you need to sign the CLA:
https://developers.google.com/open-source/cla/individual
In a follow up change, you can add yourself to AUTHORS in the Chromium
repository.

[leviw_travelin_and_unemployed, 2013-11-20 19:09:36]

On 2013/11/20 17:29:50, tony wrote:
> For those following at home, on www-style, Tab said that matching IE11 (which
> this patch does) makes sense.
Awesome, thanks for clearing that up.

> 
> This change looks correct to me, but someone more actively working on blink
> probably ought to weigh in.
> 
> I kicked off some try jobs.
The compile fails :( I'm happy to review when this is fixed. dtrebbien, please
comment on the bug or ping me when you re-upload a patch that builds and I'm
happy to kick off the try bots again :)

> 
> dtrebblen, you need to sign the CLA:
> https://developers.google.com/open-source/cla/individual
> In a follow up change, you can add yourself to AUTHORS in the Chromium
> repository.

[dtrebbien, 2013-11-20 19:11:08]

On 2013/11/20 17:29:50, tony wrote:
> For those following at home, on www-style, Tab said that matching IE11 (which
> this patch does) makes sense.
> 
> This change looks correct to me, but someone more actively working on blink
> probably ought to weigh in.
> 
> I kicked off some try jobs.
> 
> dtrebblen, you need to sign the CLA:
> https://developers.google.com/open-source/cla/individual
> In a follow up change, you can add yourself to AUTHORS in the Chromium
> repository.

Hi Tony,

I have signed the CLA.  I'm also listed in AUTHORS for some previous
contributions that have been committed
(https://src.chromium.org/viewvc/chrome?revision=212038&view=revision).

Those compile failures are unrelated, right?

[leviw_travelin_and_unemployed, 2013-11-20 19:31:12]

On 2013/11/20 19:11:08, dtrebbien wrote:
> On 2013/11/20 17:29:50, tony wrote:
> > For those following at home, on www-style, Tab said that matching IE11
(which
> > this patch does) makes sense.
> > 
> > This change looks correct to me, but someone more actively working on blink
> > probably ought to weigh in.
> > 
> > I kicked off some try jobs.
> > 
> > dtrebblen, you need to sign the CLA:
> > https://developers.google.com/open-source/cla/individual
> > In a follow up change, you can add yourself to AUTHORS in the Chromium
> > repository.
> 
> Hi Tony,
> 
> I have signed the CLA.  I'm also listed in AUTHORS for some previous
> contributions that have been committed
> (https://src.chromium.org/viewvc/chrome?revision=212038&view=revision).
> 
> Those compile failures are unrelated, right?

Yeah they do. Just kicked off some more builds.

[leviw_travelin_and_unemployed, 2013-11-20 19:33:26]

The code change looks good. Given the implications for pointer events, I'd
really like a test that covers that functionality as well.

https://codereview.chromium.org/68243009/diff/200001/LayoutTests/scrollbars/s...
File LayoutTests/scrollbars/scrollbar-visibility-hidden.html (right):

https://codereview.chromium.org/68243009/diff/200001/LayoutTests/scrollbars/s...
LayoutTests/scrollbars/scrollbar-visibility-hidden.html:4: <meta
http-equiv="content-type" content="text/html; charset=UTF-8">
Is this needed?

https://codereview.chromium.org/68243009/diff/200001/LayoutTests/scrollbars/s...
LayoutTests/scrollbars/scrollbar-visibility-hidden.html:7: <!--
http://crbug.com/313908 -->
How about just putting this in the description?

[dtrebbien, 2013-11-21 13:48:15]

On 2013/11/20 19:33:26, Levi wrote:
> The code change looks good. Given the implications for pointer events, I'd
> really like a test that covers that functionality as well.

Done.

> 
>
https://codereview.chromium.org/68243009/diff/200001/LayoutTests/scrollbars/s...
> File LayoutTests/scrollbars/scrollbar-visibility-hidden.html (right):
> 
>
https://codereview.chromium.org/68243009/diff/200001/LayoutTests/scrollbars/s...
> LayoutTests/scrollbars/scrollbar-visibility-hidden.html:4: <meta
> http-equiv="content-type" content="text/html; charset=UTF-8">
> Is this needed?

Removed.

> 
>
https://codereview.chromium.org/68243009/diff/200001/LayoutTests/scrollbars/s...
> LayoutTests/scrollbars/scrollbar-visibility-hidden.html:7: <!--
> http://crbug.com/313908 -->
> How about just putting this in the description?

Done.

[dtrebbien, 2013-11-22 12:27:48]

I thought that I could trigger try rebuilds, but it just shows up as red in
Rietveld.

Could someone add some trybots for the latest patch set?  I would like to see if
these build failures are repeatable:
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/1...
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds...
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout_rel/buil...
http://build.chromium.org/p/tryserver.chromium/builders/win_layout_rel/builds...
http://build.chromium.org/p/tryserver.chromium/builders/linux_blink/builds/3519
http://build.chromium.org/p/tryserver.chromium/builders/win_blink/builds/832

If so, I can start by setting up a Linux VM to try to figure out why
inspector/styles/styles-should-not-force-sync-style-recalc.html failed and go
from there.

[dtrebbien, 2013-12-03 23:43:02]

Just wanted to let you all know that I haven't forgotten about this :)

I am working on setting up an Ubuntu VM with a debug build, but I have been
experiencing a lot of trouble with compilation and linker errors the past
several times that I have tried to build content_shell.

[dtrebbien, 2013-12-20 23:49:07]

On 2013/11/22 12:27:48, dtrebbien wrote:
> Could someone add some trybots for the latest patch set?  I would like to see
if
> these build failures are repeatable:
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/1...
>
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds...
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout_rel/buil...
>
http://build.chromium.org/p/tryserver.chromium/builders/win_layout_rel/builds...
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_blink/builds/3519
> http://build.chromium.org/p/tryserver.chromium/builders/win_blink/builds/832
> 
> If so, I can start by setting up a Linux VM to try to figure out why
> inspector/styles/styles-should-not-force-sync-style-recalc.html failed and go
> from there.

Hi again.

I finally managed to build content_shell in an Ubuntu 13.04 'Raring Ringtail'
VM.  With or without the patch applied,
inspector/styles/styles-should-not-force-sync-style-recalc.html is passing for
me.

[ojan, 2014-01-02 23:32:24]

lgtm

[commit-bot: I haz the power, 2014-01-02 23:32:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrebbien@gmail.com/68243009/310001

[commit-bot: I haz the power, 2014-01-03 00:40:25]

Change committed as 164436