Element::shadowPseudoId should not be virtual |
||||||||
Issue descriptionThe default implementation uses the "pseudo" attribute to enable pseudo elements inside UA shadow roots. This works because they're never dynamically set so we can avoid needing to schedule any kind of invalidation. The method is also virtual though, and there's a few implementations of it that do something dynamic which doesn't work without special invalidation logic (which I don't think we should add). - MediaControlCastButtonElement::shadowPseudoId() - SliderContainerElement::shadowPseudoId() - SliderThumbElement::shadowPseudoId() The form control implementations also look at the style of the parent to dynamically decide what the pseudo element name should be which is bad because it means we can't statically know what selectors actually match this element without first computing the style of the parent which is a violation of the way selectors work in general where conceptually matching over the entire tree happens first, and then computation of styles.
,
Mar 15 2017
One patch: https://codereview.chromium.org/2754523004 The form control ones look scarier, but we should try to fix that too so we avoid the style computation ordering complexity.
,
Mar 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/91afacdd96a5d1ca0a0e3235bc62526834a67463 commit 91afacdd96a5d1ca0a0e3235bc62526834a67463 Author: esprehn <esprehn@chromium.org> Date: Wed Mar 15 06:31:10 2017 Remove dynamic MediaControlCastButtonElement::shadowPseudoId. The m_isOverlayButton bit is never set dynamically, and even if it was it wouldn't work right unless we forced a style recalc at the same time to make sure we rematched the style with the new pseudo value. Instead of having this virtual override lets just use the default one and set the pseudo attribute when the element is created. BUG= 701655 Review-Url: https://codereview.chromium.org/2754523004 Cr-Commit-Position: refs/heads/master@{#457014} [modify] https://crrev.com/91afacdd96a5d1ca0a0e3235bc62526834a67463/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp [modify] https://crrev.com/91afacdd96a5d1ca0a0e3235bc62526834a67463/third_party/WebKit/Source/core/html/shadow/MediaControlElements.h
,
Mar 17 2017
Rune - is this something you're working on?
,
Mar 17 2017
Chatted with Elliott, I understand now that this is not a 'will be worked on right now' issue.
,
Mar 19 2017
,
Mar 19 2017
,
Mar 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cadf4232009c7877216ff5485244c263193ac4f5 commit cadf4232009c7877216ff5485244c263193ac4f5 Author: esprehn <esprehn@chromium.org> Date: Tue Mar 21 03:47:29 2017 Remove the -webkit-fullscreen-volume-* pseudos, they're dead code. This was added for an ancient version of the media controls back in 2012: https://chromium.googlesource.com/chromium/src/+/01b1110b198ad023fcdf22092e570b80b2a1020a That doesn't appear to exist anymore. Removing this reduces the number of cases where ::shadowPseudoId() dynamically returns a different value. BUG= 701655 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2760063003 Cr-Commit-Position: refs/heads/master@{#458301} [modify] https://crrev.com/cadf4232009c7877216ff5485244c263193ac4f5/third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h [modify] https://crrev.com/cadf4232009c7877216ff5485244c263193ac4f5/third_party/WebKit/Source/core/css/CSSValueKeywords.json5 [modify] https://crrev.com/cadf4232009c7877216ff5485244c263193ac4f5/third_party/WebKit/Source/core/css/mediaControls.css [modify] https://crrev.com/cadf4232009c7877216ff5485244c263193ac4f5/third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h [modify] https://crrev.com/cadf4232009c7877216ff5485244c263193ac4f5/third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp [modify] https://crrev.com/cadf4232009c7877216ff5485244c263193ac4f5/third_party/WebKit/Source/core/layout/LayoutSliderThumb.cpp [modify] https://crrev.com/cadf4232009c7877216ff5485244c263193ac4f5/third_party/WebKit/Source/core/layout/LayoutTheme.cpp [modify] https://crrev.com/cadf4232009c7877216ff5485244c263193ac4f5/third_party/WebKit/Source/core/paint/ThemePainter.cpp [modify] https://crrev.com/cadf4232009c7877216ff5485244c263193ac4f5/third_party/WebKit/Source/modules/accessibility/AXMediaControls.cpp [modify] https://crrev.com/cadf4232009c7877216ff5485244c263193ac4f5/third_party/WebKit/Source/modules/accessibility/AXSlider.cpp [modify] https://crrev.com/cadf4232009c7877216ff5485244c263193ac4f5/third_party/WebKit/Source/platform/ThemeTypes.h
,
Dec 6 2017
,
Dec 6
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 10
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by esprehn@chromium.org
, Mar 15 2017