Intent to implement: Do not apply hover when mouse does not move

189 views
Skip to first unread message

Lan Wei

unread,
Sep 5, 2018, 6:27:31 PM9/5/18
to blin...@chromium.org

Contact emails

lan...@chromium.org, bokan@chromium.org, nzolghadr@chromium.org

 

Summary

Right now in Blink, in order to update the hover state when the page scrolls or the page layout is changed, we have to dispatch fake mouse moves based on a timer, because the real mouse does not move. However, every mouse move does a hit test, which is an issue for performance as well as testing. The proposal is to apply the hover effect when the mouse physically moves.

 

Motivation

The primary motivation is to improve the performance and dispatching the fake mouse events makes testing difficult and increases complexity for our developers. The hit tests of the fake mouse events really slow down the performance. In addition, whether updating the hover state when the mouse does not move is desirable from a UX standpoint is debatable, and the fake mouse events cause many issues to web developers and testing.


Interoperability and Compatibility risk

Yes. Firefox and Safari update the hover state when the page layout is changed and after scroll. Edge does not update the hover state when the page layout is changed, but it updates hover after scroll.


Will this feature be supported on all six Blink platforms (Windows, Mac, Linux,

Chrome OS, Android, and Android WebView)?

Yes


Demo link

https://output.jsbin.com/cekolep/quiet/

https://www.bitdegree.org/learn/best-code-editor/?example=26436


OWP launch tracking bug

https://crbug.com/877132


Link to entry on the Chrome Platform Status

https://www.chromestatus.com/feature/6058736774807552

 

Requesting approval to ship

no


taylorch...@gmail.com

unread,
Sep 5, 2018, 9:52:36 PM9/5/18
to blink-dev
This is something Web Devs have been interested in before: https://www.thecssninja.com/css/pointer-events-60fps

Chris Harrelson

unread,
Sep 5, 2018, 11:50:23 PM9/5/18
to Lan Wei, blink-dev
I am a big fan of this!

I think it will significantly help performance by reducing forced layouts and timers, and re-layouts to change hover style that are not relevant or redundant.
I also think it will improve UX on sites with hover styles, to avoid unintended, distracting hover styles during scroll gestures unrelated to mouse position.

Chris

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CA%2BEkLDjvH4ZNgsjSXucQq4y_R0M9BNwXRbyTj3rzrDJewaTU3A%40mail.gmail.com.

Kenji Baheux

unread,
Sep 5, 2018, 11:59:13 PM9/5/18
to Chris Harrelson, lan...@chromium.org, blink-dev
Non owner, SGTM.

Like Chris, I expect that this will improve performance.
Can we try to assess the impact (e.g. how often we avoided unnecessary work, how costly the work was)?



--
Kenji BAHEUX
Product Manager - Chrome
Google Japan

smaug

unread,
Sep 6, 2018, 4:12:34 AM9/6/18
to Lan Wei, blin...@chromium.org
Will this affect to mouseover and other similar events?
Spec about mouseover
"A user agent MUST dispatch this event when a pointing device is moved onto the boundaries of an element or when the element is moved to be underneath
the primary pointing device. "
Notice the latter part of the sentence.



-Olli


On 09/06/2018 12:02 AM, Lan Wei wrote:
> Contact emails
>
> lan...@chromium.org <mailto:lan...@chromium.org>, bokan <https://bugs.chromium.org/u/bo...@chromium.org/>@chromium.org <http://chromium.org>,
> nzolghadr <https://bugs.chromium.org/u/nzol...@chromium.org/>@chromium.org <http://chromium.org>
>
> Summary
>
> Right now in Blink, in order to update the hover state when the page scrolls or the page layout is changed, we have to dispatch fake mouse moves based
> on a timer, because the real mouse does not move. However, every mouse move does a hit test, which is an issue for performance as well as testing. The
> proposal is to apply the hover effect when the mouse physically moves.
>
> Motivation
>
> The primary motivation is to improve the performance and dispatching the fake mouse events makes testing difficult and increases complexity for our
> developers. The hit tests of the fake mouse events really slow down the performance. In addition, whether updating the hover state when the mouse does
> not move is desirable from a UX standpoint is debatable, and the fake mouse events cause many issues to web developers and testing.
>
>
> Interoperability and Compatibility risk
>
> Yes. Firefox and Safari update the hover state when the page layout is changed and after scroll. Edge does not update the hover state when the page
> layout is changed, but it updates hover after scroll.
>
>
> Will this feature be supported on all six Blink platforms (Windows, Mac, Linux,
>
> Chrome OS, Android, and Android WebView)?
>
> Yes
>
>
> Demo link
>
> https://output.jsbin.com/cekolep/quiet/
>
> https://www.bitdegree.org/learn/best-code-editor/?example=26436
>
>
> OWP launch tracking bug
>
> https://crbug.com/877132 <https://bugs.chromium.org/p/chromium/issues/detail?id=877132>
>
>
> Link to entry on the Chrome Platform Status
>
> https://www.chromestatus.com/feature/6058736774807552
>
> Requesting approval to ship
>
> no
>
>
> --
> You received this message because you are subscribed to the Google Groups "blink-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org
> <mailto:blink-dev+...@chromium.org>.
> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CA%2BEkLDjvH4ZNgsjSXucQq4y_R0M9BNwXRbyTj3rzrDJewaTU3A%40mail.gmail.com?utm_medium=email&utm_source=footer>.

Lan Wei

unread,
Sep 6, 2018, 10:22:43 AM9/6/18
to blink-dev, lan...@chromium.org, sm...@welho.com
Yes, if the mouse does not move but the element under the mouse cursor is changed, we will not send any mouseover/mouseenter events to this element.

PhistucK

unread,
Sep 6, 2018, 10:53:15 AM9/6/18
to Lan Wei, blink-dev, Lan Wei, smaug
Then this requires a specification change (before shipping).

PhistucK


To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/414aca9a-5aa1-4c3c-9532-17af868ff1fe%40chromium.org.

Navid Zolghadr

unread,
Sep 6, 2018, 11:12:46 AM9/6/18
to PhistucK, Lan Wei, blink-dev, Lan Wei, smaug
Thanks Olli for bringing that up. Specification change is definitely one part of the story if we go for shipping this.

Let us know if you have any important use case that this might break so we can have that into consideration as well. We wanted to first have this feature behind a flag to experiment with it and measure the compat issues and performance gains so we can have a better understanding of cons/pros of this change. 

Dave Tapuska

unread,
Sep 6, 2018, 11:47:34 AM9/6/18
to Navid Zolghadr, PhistucK, lan...@google.com, blink-dev, Lan Wei, smaug
Specifically I added that text to the spec so are you undoing issue 154 ?

dave.

Chris Harrelson

unread,
Sep 6, 2018, 12:36:35 PM9/6/18
to Dave Tapuska, Navid Zolghadr, PhistucK, Lan Wei, blink-dev, Lan Wei, smaug
On Thu, Sep 6, 2018 at 8:47 AM Dave Tapuska <dtap...@chromium.org> wrote:
Specifically I added that text to the spec so are you undoing issue 154 ?

Interesting. What were the motivating use cases for that text? Or was it mainly about consistency?
 

Dave Tapuska

unread,
Sep 6, 2018, 1:09:47 PM9/6/18
to Chris Harrelson, Navid Zolghadr, PhistucK, lan...@google.com, blink-dev, Lan Wei, smaug
It was related to specing this intent. I'm curious what the motivation here is. We specifically decided before that the mouse events sent should always match the hover states. Being that I've been out of the loop on input things I'm curious on the motiviation of this. Was the previous intent flawed? Where there drawbacks? Why are we deviating from Firefox here?

dave.

Chris Harrelson

unread,
Sep 6, 2018, 1:54:10 PM9/6/18
to Dave Tapuska, Navid Zolghadr, PhistucK, Lan Wei, blink-dev, Lan Wei, smaug
On Thu, Sep 6, 2018 at 10:09 AM Dave Tapuska <dtap...@chromium.org> wrote:
It was related to specing this intent. I'm curious what the motivation here is. We specifically decided before that the mouse events sent should always match the hover states. Being that I've been out of the loop on input things I'm curious on the motiviation of this. Was the previous intent flawed? Where there drawbacks? Why are we deviating from Firefox here?

To me the primary motivation is performance. If the fake mouse event timer happens when the document lifecycle is dirty, Blink has to perform a forced style and layout which otherwise would not have been necessary. Then it performs a hit test to determine the new elements under the mouse. This in turn can cause a full lifecycle when hover state changes (which may actually change hover due to layout moving elements out from the mouse right? - a rendering circularity). The link provided earlier in the thread (here) goes into some more cases of this performance problem. The hacks described there are the only way to avoid this performance problem as far as I can see. In their case, scroll is being optimized, but UIs which want to have hover state but don't use scroll for certain interactions will also suffer performance problems.

I agree that is good to have consistency in the system also. I looked at the doc and bugs linked from that intent, they cover a number of cases...I'm not sure about the best resolution, but do feel that the performance problem needs fixing.

K. York

unread,
Sep 6, 2018, 2:26:30 PM9/6/18
to blink-dev, dtap...@chromium.org, nzol...@chromium.org, phis...@gmail.com, lan...@google.com, lan...@chromium.org, sm...@welho.com


On Thursday, September 6, 2018 at 10:54:10 AM UTC-7, Chris Harrelson wrote:
On Thu, Sep 6, 2018 at 10:09 AM Dave Tapuska <dtap...@chromium.org> wrote:
It was related to specing this intent. I'm curious what the motivation here is. We specifically decided before that the mouse events sent should always match the hover states. Being that I've been out of the loop on input things I'm curious on the motiviation of this. Was the previous intent flawed? Where there drawbacks? Why are we deviating from Firefox here?

To me the primary motivation is performance. If the fake mouse event timer happens when the document lifecycle is dirty, Blink has to perform a forced style and layout which otherwise would not have been necessary. Then it performs a hit test to determine the new elements under the mouse. This in turn can cause a full lifecycle when hover state changes (which may actually change hover due to layout moving elements out from the mouse right? - a rendering circularity). The link provided earlier in the thread (here) goes into some more cases of this performance problem. The hacks described there are the only way to avoid this performance problem as far as I can see. In their case, scroll is being optimized, but UIs which want to have hover state but don't use scroll for certain interactions will also suffer performance problems.

Removing the timer-based checks and switching to a triggered model - shortly after layout changes, and on scroll quiescence + a low-res timer during scroll - makes more sense if the goal is to improve performance.

Navid Zolghadr

unread,
Sep 6, 2018, 2:58:32 PM9/6/18
to kane...@gmail.com, blink-dev, Dave Tapuska, PhistucK, Lan Wei, Lan Wei, smaug
There are a few options we are investigating. We are not planning to break the pairing of boundary events and hover state.

However, we would like to update/send them less frequently. For example instead of having timers all around maybe not update anything during scroll and once the scroll is finished we update the hover and send boundary events.

Chris Harrelson

unread,
Sep 6, 2018, 4:15:08 PM9/6/18
to kane...@gmail.com, blink-dev, Dave Tapuska, Navid Zolghadr, PhistucK, Lan Wei, Lan Wei, smaug
On Thu, Sep 6, 2018 at 11:26 AM K. York <kane...@gmail.com> wrote:


On Thursday, September 6, 2018 at 10:54:10 AM UTC-7, Chris Harrelson wrote:
On Thu, Sep 6, 2018 at 10:09 AM Dave Tapuska <dtap...@chromium.org> wrote:
It was related to specing this intent. I'm curious what the motivation here is. We specifically decided before that the mouse events sent should always match the hover states. Being that I've been out of the loop on input things I'm curious on the motiviation of this. Was the previous intent flawed? Where there drawbacks? Why are we deviating from Firefox here?

To me the primary motivation is performance. If the fake mouse event timer happens when the document lifecycle is dirty, Blink has to perform a forced style and layout which otherwise would not have been necessary. Then it performs a hit test to determine the new elements under the mouse. This in turn can cause a full lifecycle when hover state changes (which may actually change hover due to layout moving elements out from the mouse right? - a rendering circularity). The link provided earlier in the thread (here) goes into some more cases of this performance problem. The hacks described there are the only way to avoid this performance problem as far as I can see. In their case, scroll is being optimized, but UIs which want to have hover state but don't use scroll for certain interactions will also suffer performance problems.

Removing the timer-based checks and switching to a triggered model - shortly after layout changes, and on scroll quiescence + a low-res timer during scroll - makes more sense if the goal is to improve performance.

I agree that something like this could also help. In the parlance of Blink's implementation, we could do a synchronous hit test if needed at the end of / part of paint and then re-compute style/layout if needed based on the result. It'd be similar in spirit to when ResizeObservers run.
 

Boris Zbarsky

unread,
Sep 14, 2018, 2:10:48 AM9/14/18
to Lan Wei, blin...@chromium.org
On 9/5/18 5:02 PM, Lan Wei wrote:
> The proposal is to apply the hover effect when the
> mouse physically moves.

I believe Blink used to have that behavior. Worth looking into why the
current behavior was implemented, in case those reasons are still relevant.

-Boris

Rick Byers

unread,
Sep 28, 2018, 3:23:30 PM9/28/18
to Boris Zbarsky, Tim Dresser, lan...@chromium.org, blink-dev
Sorry for the long lag (catching up on old e-mail while on a plane).

I thought +Tim Dresser already changed the hover timer to fire only at the end of a scroll? I.e. I thought the performance impact should be pretty minimal with the current heuristics. Do we have any hard data on the perf benefit? Eg. we could add UMA metrics to count CPU time spent inside timer-induced mouse move handlers as a fraction of total main thread cpu time. Or maybe we have some anecdotes of this showing up a a non-trivial cost in a trace in some scenario? 

I'm not necessarily opposed to switching consistently to the other model - but we should have compelling data for trying to convince the other browsers to align and to answer the inevitable "Why am I now seeing tooltips for things my mouse is no longer over?" bugs...


--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

Timothy Dresser

unread,
Oct 1, 2018, 10:57:44 AM10/1/18
to Rick Byers, Boris Zbarsky, lan...@chromium.org, blink-dev
For wheel scrolling, we definitely only hit test if we haven't scrolled for greater than some time duration.

Lan Wei

unread,
Oct 1, 2018, 12:41:20 PM10/1/18
to Timothy Dresser, Rick Byers, Boris Zbarsky, blink-dev
When I tried to slowly wheel scroll on a page, I can see some hover updates while scrolling. I will submit for a finch experiment to see if we do have an improvement.

Peter Kasting

unread,
Oct 1, 2018, 2:32:54 PM10/1/18
to lan...@chromium.org, blink-dev
On Wed, Sep 5, 2018 at 3:27 PM Lan Wei <lan...@chromium.org> wrote:

Motivation

In addition, whether updating the hover state when the mouse does not move is desirable from a UX standpoint is debatable,


As a non-Blink UX person -- updating the hover state when the pages scrolls or does layout, but the mouse does not physically move, is desirable.  The proposed behavior is a UX regression.

For example, hover state is often used to indicate what will happen on click, e.g. by changing the cursor.  When the clickable item changes, the hover state needs to change in sync.

As a comparison point, see Windows native controls, which update the hovered element when wheel-scrolling without moving the mouse (try a list of files in the file explorer).  I believe (but don't have a good testcase at the moment) that the views UI toolkit we build top chrome with also tries to update hover state in these sorts of cases.

PK

Lan Wei

unread,
Nov 7, 2018, 2:46:32 PM11/7/18
to Peter Kasting, blink-dev
We have submitted the changes that we will not dispatch the fake mouse move based on a timer, but we will update the hover state once the scroll finishes.

Should we enable the NoHoverDuringScrollEnabled flag by default, do we need an intent to ship, we did not change any behavior of webpages from web users?

David Bokan

unread,
Nov 7, 2018, 3:28:54 PM11/7/18
to blink-dev, pkas...@chromium.org
(I replied from the wrong e-mail but it's awaiting approval so I'm replying again. Apologies if this appears twice)


On Wednesday, November 7, 2018 at 2:46:32 PM UTC-5, Lan Wei wrote:
We have submitted the changes that we will not dispatch the fake mouse move based on a timer, but we will update the hover state once the scroll finishes.

To clarify, this change will change the timer-based approach of update hover to a synchronous one, similar to what Chris described earlier in the thread, for scrolling-only (style/layout changes to come later). This doesn't really change web-exposed behavior in a real way so I expect we don't need an intent-to-ship. Do API owners agree?

I'm a bit late here, I missed the earlier discussion. I notice it focused mainly on performance changes. I expect this proposal won't have a significant effect on performance. The main benefit comes from better predictability and reduced complexity.

bo...@google.com

unread,
Nov 7, 2018, 4:58:55 PM11/7/18
to blink-dev, pkas...@chromium.org
On Wednesday, November 7, 2018 at 2:46:32 PM UTC-5, Lan Wei wrote:
We have submitted the changes that we will not dispatch the fake mouse move based on a timer, but we will update the hover state once the scroll finishes.

To clarify - this is for updating hover state on scrolling only (not layout/style based changes).  The changes above will remove the timer based approach in favor of a synchronous one similar to what Chris mentioned above. Given there's no real web-facing impact I expect that an intent-to-ship isn't necessary.

Also, I missed this discussion earlier and it looks like it was really focused on performance improvement. I don't expecct the proposed changes will have a noticeable impact on performance. IMHO, the main motivation here is simplicity and predictability. 

Elliott Sprehn

unread,
Nov 7, 2018, 10:51:22 PM11/7/18
to blink-dev, pkas...@chromium.org
The intent and the discussion in the bug makes it sound like you want to remove the timer entirely for all style and layout changes. The RuntimeEnabledFeatures::NoHoverAfterLayoutChangeEnabled() flag currently also disables the update and the timer entirely not just for scroll.

The update is needed so that mouseout (and other mouse/pointer events) are fired when the content under the mouse changes. For example if I hover an item in a search list a hover card might appear, if the search list then changes the hover card should disappear. Not firing any event would break existing UX depending on this and authors couldn't easily implement this effect (in theory using IntersectionObserver works too, but it means duplicated code). Native toolkits also tell you if the widget under the mouse vanishes even if the mouse didn't move.

The timer is needed because it's not safe to run script inside style or layout, so when the element under the mouse changes a timer is used to safely dispatch the events later. It also provides natural queuing because thrashy style or layout only triggers one hit test.

To Chris's comments about performance I think fixing RenderWidget::UpdateTextInputState() to not cause a synchronous style+layout update would be a bigger impact. In BeginMainFrame before raf callbacks, scroll handlers and resize event handlers run we call into LayerTreeView::WillBeginMainFrame() -> WillBeginCompositorFrame() which then forces a style+layout update. This both breaks transitions in some situations (and probably causes web compat issues), but is also a perf issue since scroll handlers and raf callbacks will dirty layout again immediately after. I've seen this on a number of sites in the past where an <input> causes a layout flush *before* raf every frame.

I don't think RuntimeEnabledFeatures::NoHoverAfterLayoutChangeEnabled() is a good idea from a UX perspective, and firing the events synchronously isn't safe.

- E 

bo...@google.com

unread,
Nov 8, 2018, 1:09:34 PM11/8/18
to blink-dev, pkas...@chromium.org
On Wednesday, November 7, 2018 at 10:51:22 PM UTC-5, Elliott Sprehn wrote:
The intent and the discussion in the bug makes it sound like you want to remove the timer entirely for all style and layout changes. The RuntimeEnabledFeatures::NoHoverAfterLayoutChangeEnabled() flag currently also disables the update and the timer entirely not just for scroll.

There are two flags as part of this change, one is the style+layout hover update, NoHoverAfterLayoutChangeEnabled. The other is scroll hover update behind NoHoverDuringScrollEnabled. The proposed non-intent-to-ship change above is for the scrolling case only, synchronously performing the hover update in a GestureScrollEnd should be ok. One thing I hadn't thought of earlier though is the flag doesn't yet work for programmatic scrolls and non-gesture scrolls like keyboard so we'd want to implement that before shipping it.

The timer is needed because it's not safe to run script inside style or layout, so when the element under the mouse changes a timer is used to safely dispatch the events later. It also provides natural queuing because thrashy style or layout only triggers one hit test.

The intent is to do it at a safe time inside the lifecycle, not inside style/layout itself. JS execution already happens inside the lifecycle after a layout, for example, see: Document::LayoutUpdated. Perhaps it'd make sense to queue the DOM events until the next raf.

The difficulty with using timers/postTask for this is it makes things like tests less predictable and harder to write (IIRC, pdr@ was bitten by this recently which spurred this project). It also causes scheduling surprises, e.g. 855576. These would just go away if we integrated better with the lifecycle.
 
I don't think RuntimeEnabledFeatures::NoHoverAfterLayoutChangeEnabled() is a good idea from a UX perspective, and firing the events synchronously isn't safe.

Agreed, we don't want to change the existing UX.

Elliott Sprehn

unread,
Nov 9, 2018, 4:50:56 PM11/9/18
to David Bokan, blin...@chromium.org, Peter Kasting
On Thu, Nov 8, 2018 at 10:09 AM bokan via blink-dev <blin...@chromium.org> wrote:
On Wednesday, November 7, 2018 at 10:51:22 PM UTC-5, Elliott Sprehn wrote:
The intent and the discussion in the bug makes it sound like you want to remove the timer entirely for all style and layout changes. The RuntimeEnabledFeatures::NoHoverAfterLayoutChangeEnabled() flag currently also disables the update and the timer entirely not just for scroll.

There are two flags as part of this change, one is the style+layout hover update, NoHoverAfterLayoutChangeEnabled. The other is scroll hover update behind NoHoverDuringScrollEnabled. The proposed non-intent-to-ship change above is for the scrolling case only, synchronously performing the hover update in a GestureScrollEnd should be ok. One thing I hadn't thought of earlier though is the flag doesn't yet work for programmatic scrolls and non-gesture scrolls like keyboard so we'd want to implement that before shipping it.

The timer is needed because it's not safe to run script inside style or layout, so when the element under the mouse changes a timer is used to safely dispatch the events later. It also provides natural queuing because thrashy style or layout only triggers one hit test.

The intent is to do it at a safe time inside the lifecycle, not inside style/layout itself. JS execution already happens inside the lifecycle after a layout, for example, see: Document::LayoutUpdated. Perhaps it'd make sense to queue the DOM events until the next raf.


The ScriptForbiddenScope was there to guard against system sanity and spec violations. The timers and async post layout tasks are required to maintain invariants and spec compliance. Previously only plugins could run script inside layout and it was considered a bug. Plugins also don't exist on mobile, and flash is on the way out. I think we also block the script in plugins in a bunch of situations so those comments are probably out of date.

That change also breaks some specs. For example that change means that events dispatch inside .offsetTop now which isn't allowed, and is very confusing as an author. It also means bad frames are painted when requestAnimationFrame() is called inside a relevant event handler. For example: onfocus = () => { body.style.color = 'red'; requestAnimationFrame(() => body.style.color = 'blue') } that must paint blue per spec, but it'll paint red for one frame with the change that runs script inside layout. Raf callbacks must always be last.

I understand the timers are annoying, but I think you probably want to revert those changes and expand the ScriptForbiddenScope to cover the whole function again. The change discussed in https://bugs.chromium.org/p/chromium/issues/detail?id=796222 is not safe or spec compliant.
 

The difficulty with using timers/postTask for this is it makes things like tests less predictable and harder to write (IIRC, pdr@ was bitten by this recently which spurred this project). It also causes scheduling surprises, e.g. 855576. These would just go away if we integrated better with the lifecycle.
 
I don't think RuntimeEnabledFeatures::NoHoverAfterLayoutChangeEnabled() is a good idea from a UX perspective, and firing the events synchronously isn't safe.

Agreed, we don't want to change the existing UX.

Great!

- E

David Bokan

unread,
Dec 10, 2018, 7:02:34 PM12/10/18
to blink-dev, bo...@google.com, pkas...@chromium.org
Having given this some more detailed thought, I've outlined how we can implement a timerless version of the hover update. This won't have a UX change but should be an improvement in terms of lifecycle architecture, PTAL at the doc. Comments welcome. 


On Friday, November 9, 2018 at 4:50:56 PM UTC-5, Elliott Sprehn wrote:


On Thu, Nov 8, 2018 at 10:09 AM bokan via blink-dev <blin...@chromium.org> wrote:
On Wednesday, November 7, 2018 at 10:51:22 PM UTC-5, Elliott Sprehn wrote:
The intent and the discussion in the bug makes it sound like you want to remove the timer entirely for all style and layout changes. The RuntimeEnabledFeatures::NoHoverAfterLayoutChangeEnabled() flag currently also disables the update and the timer entirely not just for scroll.

There are two flags as part of this change, one is the style+layout hover update, NoHoverAfterLayoutChangeEnabled. The other is scroll hover update behind NoHoverDuringScrollEnabled. The proposed non-intent-to-ship change above is for the scrolling case only, synchronously performing the hover update in a GestureScrollEnd should be ok. One thing I hadn't thought of earlier though is the flag doesn't yet work for programmatic scrolls and non-gesture scrolls like keyboard so we'd want to implement that before shipping it.

The timer is needed because it's not safe to run script inside style or layout, so when the element under the mouse changes a timer is used to safely dispatch the events later. It also provides natural queuing because thrashy style or layout only triggers one hit test.

The intent is to do it at a safe time inside the lifecycle, not inside style/layout itself. JS execution already happens inside the lifecycle after a layout, for example, see: Document::LayoutUpdated. Perhaps it'd make sense to queue the DOM events until the next raf.


The ScriptForbiddenScope was there to guard against system sanity and spec violations. The timers and async post layout tasks are required to maintain invariants and spec compliance. Previously only plugins could run script inside layout and it was considered a bug. Plugins also don't exist on mobile, and flash is on the way out. I think we also block the script in plugins in a bunch of situations so those comments are probably out of date.

That change also breaks some specs. For example that change means that events dispatch inside .offsetTop now which isn't allowed, and is very confusing as an author. It also means bad frames are painted when requestAnimationFrame() is called inside a relevant event handler. For example: onfocus = () => { body.style.color = 'red'; requestAnimationFrame(() => body.style.color = 'blue') } that must paint blue per spec, but it'll paint red for one frame with the change that runs script inside layout. Raf callbacks must always be last.

I understand the timers are annoying, but I think you probably want to revert those changes and expand the ScriptForbiddenScope to cover the whole function again. The change discussed in https://bugs.chromium.org/p/chromium/issues/detail?id=796222 is not safe or spec compliant.

Thanks for the explanation, I'll undo that change shortly (have been strapped for time lately though). 

Thanks,
David
Reply all
Reply to author
Forward
0 new messages