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

Issue 782407 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 845944

Blocking:
issue 845404



Sign in to add a comment

Make CSSOM APIs append rather than replace slots in the declaration block.

Project Member Reported by emilio@chromium.org, Nov 7 2017

Issue description

Otherwise logical properties work inconsistently with them.

See https://github.com/w3c/csswg-drafts/issues/1898 for more background and the CSSWG resolution.
 

Comment 1 by shend@chromium.org, Nov 8 2017

Labels: Hotlist-Interop

Comment 2 by shend@chromium.org, Nov 8 2017

Labels: Update-Quarterly

Comment 3 by nainar@chromium.org, Nov 28 2017

Cc: nainar@chromium.org
Labels: Hotlist-GoodFirstBug
Assuming there is a way to let us optimize for redundant setters - this should be a good bug for someone to take on. 

Here is the code: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/StylePropertySet.cpp?gsn=SetProperty&l=347 
for the change. 
Labels: -Update-Quarterly

Comment 5 by emilio@chromium.org, May 15 2018

Owner: emilio@chromium.org
Status: Started (was: Available)
Project Member

Comment 6 by bugdroid1@chromium.org, May 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d45b94d610d27094ccfd07d20b7283813fa0bf6

commit 9d45b94d610d27094ccfd07d20b7283813fa0bf6
Author: Emilio Cobos Álvarez <emilio@chromium.org>
Date: Sat May 19 22:04:07 2018

[cssom] Make CSSOM APIs always append to the declaration block.

...instead of replacing if the property is already set.

This matches the spec[1] which was changed as a resolution from[2].

Intent thread: https://groups.google.com/a/chromium.org/d/msg/blink-dev/lzBoa1EAQpI/strfpNczAwAJ

[1]: https://drafts.csswg.org/cssom/#set-a-css-declaration
[2]: https://github.com/w3c/csswg-drafts/issues/1898

Bug: 782407
Change-Id: I4b72c0207a4cf734fc2e415bd62c7863103ec309
Reviewed-on: https://chromium-review.googlesource.com/1054867
Commit-Queue: Emilio Cobos Álvarez <emilio@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560180}
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/WebKit/LayoutTests/editing/pasteboard/dragstart-contains-default-content-expected.txt
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/WebKit/LayoutTests/editing/pasteboard/onpaste-text-html-expected.txt
[delete] https://crrev.com/ec82e04d6e4c45f133807224142330d441f4c54f/third_party/WebKit/LayoutTests/external/wpt/css/cssom/cssstyledeclaration-setter-order-expected.txt
[delete] https://crrev.com/ec82e04d6e4c45f133807224142330d441f4c54f/third_party/WebKit/LayoutTests/fast/css/no-recalc-on-no-op-inline-style-changes.html
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/WebKit/LayoutTests/fast/dom/MutationObserver/observe-attributes-expected.txt
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/WebKit/LayoutTests/fast/dom/MutationObserver/observe-attributes.html
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/WebKit/LayoutTests/fast/dom/css-set-property-exception-expected.txt
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/WebKit/LayoutTests/fast/events/ondrop-text-html-expected.txt
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/blink/renderer/core/css/css_property_value_set.cc
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/blink/renderer/core/css/css_property_value_set.h
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/blink/renderer/core/css/dom_window_css.cc
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/blink/renderer/core/css/parser/css_parser.cc
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/blink/renderer/core/css/parser/css_parser.h
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/blink/renderer/core/css/parser/css_parser_impl.cc
[modify] https://crrev.com/9d45b94d610d27094ccfd07d20b7283813fa0bf6/third_party/blink/renderer/core/css/parser/css_parser_impl.h

Comment 7 by emilio@chromium.org, May 20 2018

Status: Fixed (was: Started)
Project Member

Comment 8 by bugdroid1@chromium.org, May 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/710b5cc7dbc9d27d33fd4aa363b0b613c8a0803f

commit 710b5cc7dbc9d27d33fd4aa363b0b613c8a0803f
Author: Emilio Cobos Álvarez <emilio@chromium.org>
Date: Mon May 21 16:11:46 2018

Cleanup no longer used argument in MutableCSSPropertyValueSet::SetProperty.

I forgot to do this in the previous patch for issue 782407.

Looks like the only caller that passed it really wants the new behavior, plus
before the original patch for issue 782407 it was already kinda broken with
custom properties...

No new tests, since there's no behavior change.

Bug: 782407
Change-Id: I410ed1481256b18886c5804f4cbecde9846f2a26
Reviewed-on: https://chromium-review.googlesource.com/1066062
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Emilio Cobos Álvarez <emilio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560279}
[modify] https://crrev.com/710b5cc7dbc9d27d33fd4aa363b0b613c8a0803f/third_party/blink/renderer/core/css/css_property_value_set.cc
[modify] https://crrev.com/710b5cc7dbc9d27d33fd4aa363b0b613c8a0803f/third_party/blink/renderer/core/css/css_property_value_set.h

Comment 9 by emilio@chromium.org, May 23 2018

Blockedon: 845944
Cc: emilio@chromium.org futhark@chromium.org
Owner: ----
Status: Available (was: Fixed)
Patch was reverted in https://chromium.googlesource.com/chromium/src/+/85f8362c2b95986a4d7e574628ee8a4db33d62c1
Cc: donnd@chromium.org
The change in #6 is also the probable cause of issue 845404 where tap gestures stopped triggering Contextual Search on Google search-result pages.  Thanks for the revert -- now that issue stopped happening.  The tap on plain text was causing a CSS change (some element was changing css to display:none), which prevented Contextual Search from triggering.  If you'd like to know more details let me know. Emilio, I'll cc you on that bug.

Please reach out to me (donnd@chromium.org) when attempting to reland this so we can check if this is still a problem.  Thanks!
Hi! Could I get some more information (either via here or privately) about what the page was trying to do?

Was it observing / reacting to attribute mutations via MutationObserver? How did it detect the change in the page?

Thank you!
Blocking: 845404
Emilio, please request to merge the revert in #10 into M-68.  This is blocking issue 845404.
Emilio, in short the GestureManager checks if CSS styles change in response to a tap gesture before notifying Contextual Search here: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/input/gesture_manager.cc?type=cs&q=gesture_manager+style_changed&sq=package:chromium&g=0&l=309.  I'm not sure what part of the SERP was actually changing -- it took a while to debug and I saw a lot of shifting behavior (as you can see in that bug).  Please IM me so I can get your LDAP and so we can discuss in more detail.  Thanks!
Re. #14: There's a merge request for the revert in  issue 845944 .

Re. #15: I see, looks like that check doesn't really want to check StyleVersion(), but whether any styles have actually changed... I'm not a googler, so I don't have LDAP I presume :/.

In any case, the reason for the regression and the shifting behavior is because StyleVersion() gets incremented even if no style in the page actually has changed (only if it could have). This change made StyleVersion() to get incremented more often.
There's no guarantee that any computed styles changed with increasing StyleVersion().

Sign in to add a comment