Remove HTML.css style rule for [popover][anchor] [chromium/src : main]
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Auto-Submit | +1 |
Related details
- Joey Arhar
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted![]( "Open in Gerrit")
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Auto-Submit | +1 |
1 comment
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Auto-Submit | +1 |
2 comments
Alright, +jarhar again to review.
+dbertoni for the page_capture_api test.
// TODO(crbug.com/326868086): Test is flaky.see https://issues.chromium.org/issues/326868086#comment10
Even patchset 9 which was minimal caused these failures. I think it must be some race condition somewhere, but seemingly completely unrelated to this CL.
Related details
- David Bertoni
- Joey Arhar
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
1 comment
if (RuntimeEnabledFeatures::CSSAnchorPositioningEnabled()) {If HTMLAnchorAttribute can't be enabled without CSSAnchorPositioning then I'm not sure what this does, but I guess it doesn't hurt
Related details
- David Bertoni
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 2 comments![]( "Open in Gerrit")
Thanks jarhar!
dbertoni, this patch is a fix for a RBB bug related to media elements. hopefully you’re ok with disabling the flaky test to get it landed? I need to merge this back to the Beta branch.
if (RuntimeEnabledFeatures::CSSAnchorPositioningEnabled()) {If HTMLAnchorAttribute can't be enabled without CSSAnchorPositioning then I'm not sure what this does, but I guess it doesn't hurt
why can’t it be enabled without CSSAnchorPositioning enabled?
Related details
- David Bertoni
- Joey Arhar
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::CSSAnchorPositioningEnabled()) {Mason FreedIf HTMLAnchorAttribute can't be enabled without CSSAnchorPositioning then I'm not sure what this does, but I guess it doesn't hurt
why can’t it be enabled without CSSAnchorPositioning enabled?
Oh I thought there was a relationship set up in between them in runtime_enabled_features, but I see that there isn't so nevermind
Related details
- David Bertoni
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bertoni (Gerrit)
David Bertoni added 1 comment![]( "Open in Gerrit")
// TODO(crbug.com/326868086): Test is flaky.see https://issues.chromium.org/issues/326868086#comment10
Even patchset 9 which was minimal caused these failures. I think it must be some race condition somewhere, but seemingly completely unrelated to this CL.
Hi Mason,
I'm going to disable this test in a separate CL that's now in the CQ:
https://chromium-review.googlesource.com/c/chromium/src/+/5465997
I think that makes more sense than having you disable it in this CL.
Dave
Related details
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
// TODO(crbug.com/326868086): Test is flaky.David Bertonisee https://issues.chromium.org/issues/326868086#comment10
Even patchset 9 which was minimal caused these failures. I think it must be some race condition somewhere, but seemingly completely unrelated to this CL.
Hi Mason,
I'm going to disable this test in a separate CL that's now in the CQ:
https://chromium-review.googlesource.com/c/chromium/src/+/5465997
I think that makes more sense than having you disable it in this CL.
Dave
Thanks, and I suppose that makes sense. But would you mind please +1’ing this one as-is, with the hope that it cleanly rebases on top of yours? I’m OOO until Monday so I can’t undo that and get fresh +1’s until Monday otherwise. And then wait for Tuesday Canary to test, and then merge. This still might not cleanly merge, so that might happen anyway, but I’d like to try.
Related details
- David Bertoni
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bertoni (Gerrit)
David Bertoni voted Code-Review+1![]( "Open in Gerrit")
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bertoni (Gerrit)
David Bertoni added 1 comment![]( "Open in Gerrit")
// TODO(crbug.com/326868086): Test is flaky.David Bertonisee https://issues.chromium.org/issues/326868086#comment10
Even patchset 9 which was minimal caused these failures. I think it must be some race condition somewhere, but seemingly completely unrelated to this CL.
Mason FreedHi Mason,
I'm going to disable this test in a separate CL that's now in the CQ:
https://chromium-review.googlesource.com/c/chromium/src/+/5465997
I think that makes more sense than having you disable it in this CL.
Dave
Thanks, and I suppose that makes sense. But would you mind please +1’ing this one as-is, with the hope that it cleanly rebases on top of yours? I’m OOO until Monday so I can’t undo that and get fresh +1’s until Monday otherwise. And then wait for Tuesday Canary to test, and then merge. This still might not cleanly merge, so that might happen anyway, but I’d like to try.
Acknowledged
Related details
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Remove HTML.css style rule for [popover][anchor]
That should have been flag-guarded but wasn't. This removes that
rule entirely. After landing this and merging to M125, I'll take
care of putting that rule back, behind the flag.
This also changes the media control popup to not use the `anchor`
attribute, and instead use a named anchor. That was also broken
in the M125 shipping case, where HTMLAnchorElement is disabled,
but CSSAnchorPositioning is enabled.
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/core/html/resources/html.css
- M third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.cc
- M third_party/blink/renderer/modules/media_controls/resources/mediaControls.css
- M third_party/blink/web_tests/TestExpectations
Code-Review: +1 by David Bertoni, +1 by Joey Arhar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |