New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 754902 link

Starred by 1 user
Status: WontFix
Owner:
█ r...@opera.com
NOT IN USE
Closed: Aug 2017
Cc:
█ r...@opera.com
style-bugs@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

internals.updateStyleAndReturnAffectedElementCount() seems ill-designed

Project Member Reported by xiaoche...@chromium.org, Aug 11 2017 
This API performs a synchronous style&layout update, and counts the number of re-styled elements during the update.

However, in layout tests, the API is used to measure the number of elements whose styles are invalidated in some other operation. For example, layout test fast/css/invalidation/hover-first-letter-sibling.html does the following things:

1. Move mouse over some element, activating some CSS rules, and hence invalidating the style of a bunch of elements
2. Use the API to count the number of elements whose styles are invalidated

However, the API fails its purpose if the mouse-move operation itself performs synchronous style&layout update.

Note: this issue is found in patch crrev.com/c/611241, which adds a synchronous layout update to EventHandler::ShouldShowIBeamForNode() (so that we don't access layout tree with dirty layout), and makes some layout tests using the API fail.

Comment 1 by r...@opera.com, Aug 14 2017 

Implementing an internals.styleForElementCount() directly mapped to StyleEngine::StyleForElementCount() and let the tests force style/layout updates instead could be an option.

Comment 2 by r...@opera.com, Aug 14 2017 

That being said, does ShouldShowIBeamForNode() need to be done synchronously? Can't it wait until the next frame?

Comment 3 by r...@opera.com, Aug 14 2017 

I also found  issue 668758 , which suggest we should move certain other mouse/cursors related updates from timers to frame generation.

Comment 4 by xiaoche...@chromium.org, Aug 14 2017 

Yeah, I think deferring the handling of these events to frame generation is a better idea.

I'd better not change ShouldShowIBeamForNode() for now. The function can be called with dirty layout, which looks dangerous, but no bug has been reported about it yet. I'd better not change it in case of regressions.

Comment 5 by rjwright@chromium.org, Aug 16 2017

Owner: r...@opera.com
Status: Assigned (was: Untriaged)
Hi Rune,

I needed to triage this one since it came up in our queue, and I wasn't sure what to do with it. Assigning to you seemed most appropriate. Feel free to change the status if that isn't right.

Comment 6 by r...@opera.com, Aug 16 2017

Status: WontFix (was: Assigned)
I'm wontfixing this for now. We should try to limit internals.* apis for testing. This type of internals can be tested with unit tests where we have more control over lifecycle updates as well. We already do some of that for instance in AffectedByPseudoTest, StyleEngineTest, and more.
►

Sign in to add a comment