[dom] Introduce HyperlinkElementUtils for hyperlink elements [chromium/src : main]
Frédéric Wang Nélar (Gerrit)
Frédéric Wang Nélar voted and added 3 comments![]( "Open in Gerrit")
Votes added by Frédéric Wang Nélar
| Code-Review | -1 |
3 comments
Sounds good, but let's make sure we have things guarded (or the code owner says we don't need it) before landing. Also wait the WPT tests to be synced
HTMLAreaElement includes HyperlinkElementUtils;Frédéric Wang NélarOne failing test is
external/wpt/html/semantics/text-level-semantics/historical.html
which is fixed in
https://github.com/web-platform-tests/wpt/pull/60441
What should I do to make sure the test pass in this CL?
This landed 2 days ago in WPT (exported automatically from Firefox), I guess it will be imported back automatically in Chromium soon, so we just need to wait a bit...
HTMLAreaElement includes HyperlinkElementUtils;Frédéric Wang NélarThere are some behavior changes in HTMLAreaElement actually.
Since we add hreflang and type attribute in HyperlinkElementUtils.
No sure how to guard it behind a feature flag.
I'm not sure it's actually possible to do that, but worth asking on chromium slack if someone knows. Maybe what you can do instead is to do introduce a `HTMLAreaElementHyperlinkElementUtils` mixin in third_party/blink/renderer/core/html/html_hyperlink_element_utils.idl that has all attributes but type and hreflang attributes, make HTMLAreaElement include it here and enable type/hreflang attributes conditionally in HTMLAreaElement above. That's not nice because we are still duplicating HyperlinkElementUtils somehow, but this duplication will be remove once we ship the change (please add comments in the IDL explaining that plan)
Also, please prepare an intent-to-ship as we need one in any case. It's possible code owner will say the feature is trivial enough so it does not need to be guarded, but I don't know.
Related details
- tannal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
tannal (Gerrit)
tannal added 1 comment![]( "Open in Gerrit")
Frédéric Wang NélarThere are some behavior changes in HTMLAreaElement actually.
Since we add hreflang and type attribute in HyperlinkElementUtils.
No sure how to guard it behind a feature flag.
I'm not sure it's actually possible to do that, but worth asking on chromium slack if someone knows. Maybe what you can do instead is to do introduce a `HTMLAreaElementHyperlinkElementUtils` mixin in third_party/blink/renderer/core/html/html_hyperlink_element_utils.idl that has all attributes but type and hreflang attributes, make HTMLAreaElement include it here and enable type/hreflang attributes conditionally in HTMLAreaElement above. That's not nice because we are still duplicating HyperlinkElementUtils somehow, but this duplication will be remove once we ship the change (please add comments in the IDL explaining that plan)
Also, please prepare an intent-to-ship as we need one in any case. It's possible code owner will say the feature is trivial enough so it does not need to be guarded, but I don't know.
I inlined the attributes of HyperlinkElementUtils for HTMLAreaElement.
Guarded type and hreflang attributes with a feature flag.
Related details
- Frédéric Wang Nélar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frédéric Wang Nélar (Gerrit)
Frédéric Wang Nélar voted and added 3 comments![]( "Open in Gerrit")
Votes added by Frédéric Wang Nélar
| Code-Review | -1 |
Looks good, please send and intent to ship to blink-dev and see what code owners think.
readonly attribute USVString origin;Can you please open a new bug to track removing this duplication and add a comment
// HyperlinkElementUtils members
// FIXME(bug XXXX): Replace this with "HTMLAreaElement includes HyperlinkElementUtils" when the HTMLAreaHreflangType is on by default.
with XXXX being the id of the new bug.
// HTMLAreaElement includes HyperlinkElementUtils;I guess you can remove it for now.
Related details
- tannal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
tannal (Gerrit)
tannal added 3 comments![]( "Open in Gerrit")
Looks good, please send and intent to ship to blink-dev and see what code owners think.
Sending intent to ship blocks on the chromestatus access for now.
Can you please open a new bug to track removing this duplication and add a comment
// HyperlinkElementUtils members
// FIXME(bug XXXX): Replace this with "HTMLAreaElement includes HyperlinkElementUtils" when the HTMLAreaHreflangType is on by default.with XXXX being the id of the new bug.
Done
// HTMLAreaElement includes HyperlinkElementUtils;I guess you can remove it for now.
- Frédéric Wang Nélar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
tannal (Gerrit)
tannal added 1 comment![]( "Open in Gerrit")
Intent to ship link:
https://groups.google.com/a/chromium.org/g/blink-dev/c/FEeoLfm2yg4
Related details
- Frédéric Wang Nélar
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frédéric Wang Nélar (Gerrit)
Frédéric Wang Nélar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Frédéric Wang Nélar
| Code-Review | +1 |
1 comment
LGTM. Can you please put the link to the intent-to-ship in the commit description?
Related details
- Stephen Chenney
- tannal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
tannal (Gerrit)
tannal added 1 comment![]( "Open in Gerrit")
LGTM. Can you please put the link to the intent-to-ship in the commit description?
Done
Related details
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Chenney (Gerrit)
Stephen Chenney voted and added 3 comments![]( "Open in Gerrit")
Votes added by Stephen Chenney
| Code-Review | +1 |
3 comments
LGTM but please improve the commit message before landing.
Add hreflang and type attribute to html area element"... behind a flag, currently enabled for test"
I was confused initially about what this does. The title is correct but the description talks about the area element. I think you need the very fist line to say "Split common properties for link elements <a>, <svg> and <area> into a mixin. This is in preparation for aligning <area> with the other link elements."
Now both HTMLAnchorElement and HTMLAreaElement share the same HyperlinkElementUtils IDL mixin."When the flag is enabled both HTMLAnchorElement ..."
Related details
- tannal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
tannal (Gerrit)
tannal added 2 comments![]( "Open in Gerrit")
"... behind a flag, currently enabled for test"
I was confused initially about what this does. The title is correct but the description talks about the area element. I think you need the very fist line to say "Split common properties for link elements <a>, <svg> and <area> into a mixin. This is in preparation for aligning <area> with the other link elements."
Done
Now both HTMLAnchorElement and HTMLAreaElement share the same HyperlinkElementUtils IDL mixin."When the flag is enabled both HTMLAnchorElement ..."
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frédéric Wang Nélar (Gerrit)
Frédéric Wang Nélar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Frédéric Wang Nélar
| Code-Review | +1 |
1 comment
HTMLAreaElement includes HyperlinkElementUtils;Frédéric Wang NélarOne failing test is
external/wpt/html/semantics/text-level-semantics/historical.html
which is fixed in
https://github.com/web-platform-tests/wpt/pull/60441
What should I do to make sure the test pass in this CL?
This landed 2 days ago in WPT (exported automatically from Firefox), I guess it will be imported back automatically in Chromium soon, so we just need to wait a bit...
can you please rebase your patch and CQ dry run before we try landing it?
Related details
- tannal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
tannal (Gerrit)
tannal added 1 comment![]( "Open in Gerrit")
HTMLAreaElement includes HyperlinkElementUtils;Frédéric Wang NélarOne failing test is
external/wpt/html/semantics/text-level-semantics/historical.html
which is fixed in
https://github.com/web-platform-tests/wpt/pull/60441
What should I do to make sure the test pass in this CL?
Frédéric Wang NélarThis landed 2 days ago in WPT (exported automatically from Firefox), I guess it will be imported back automatically in Chromium soon, so we just need to wait a bit...
can you please rebase your patch and CQ dry run before we try landing it?
- Frédéric Wang Nélar
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frédéric Wang Nélar (Gerrit)
Frédéric Wang Nélar added 2 comments![]( "Open in Gerrit")
thanks
HTMLAreaElement includes HyperlinkElementUtils;Frédéric Wang NélarOne failing test is
external/wpt/html/semantics/text-level-semantics/historical.html
which is fixed in
https://github.com/web-platform-tests/wpt/pull/60441
What should I do to make sure the test pass in this CL?
Frédéric Wang NélarThis landed 2 days ago in WPT (exported automatically from Firefox), I guess it will be imported back automatically in Chromium soon, so we just need to wait a bit...
tannalcan you please rebase your patch and CQ dry run before we try landing it?
Yeah, done.
- Stephen Chenney
- tannal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Chenney (Gerrit)
Stephen Chenney voted Code-Review+1![]( "Open in Gerrit")
- tannal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frédéric Wang Nélar (Gerrit)
Frédéric Wang Nélar voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Frédéric Wang Nélar (Gerrit)
Related details
- tannal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frédéric Wang Nélar (Gerrit)
Frédéric Wang Nélar voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[dom] Introduce HyperlinkElementUtils for hyperlink elements
Split common properties for link elements <a>, <svg> and <area> into a mixin. This is in preparation for aligning <area> with the other link elements.
Add hreflang and type attributes to html area element behind a flag, currently enabled for test.
When the flag is enabled both HTMLAnchorElement and HTMLAreaElement share the same HyperlinkElementUtils IDL mixin.
See https://github.com/whatwg/html/pull/12446
I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/FEeoLfm2yg4
- M android_webview/test/data/web_tests/webexposed/global-interface-listing-expected.txt
- M third_party/blink/renderer/bindings/idl_in_core.gni
- M third_party/blink/renderer/core/html/html_anchor_element.idl
- M third_party/blink/renderer/core/html/html_area_element.idl
- M third_party/blink/renderer/core/html/html_hyperlink_element_utils.idl
- A third_party/blink/renderer/core/html/hyperlink_element_utils.idl
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/external/wpt/html/dom/idlharness.https_include=HTML.+-expected.txt
- D third_party/blink/web_tests/external/wpt/html/dom/reflection-embedded-expected.txt
- M third_party/blink/web_tests/webexposed/element-instance-property-listing-expected.txt
- M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
Code-Review: +1 by Stephen Chenney, +1 by Frédéric Wang Nélar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
luci-bisection@appspot.gserviceaccount.com (Gerrit)
Message from luci-bi...@appspot.gserviceaccount.com![]( "Open in Gerrit")
LUCI Bisection has identified this change as the cause of a test failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/test-analysis/b/5205535692423168
Sample build with failed test: https://ci.chromium.org/b/8679042222380673825
Affected test(s):
[://\:chrome_wpt_tests!webtest::external/wpt/html/dom#idlharness.https.html?include=HTML.+](https://ci.chromium.org/ui/test/chromium/:%2F%2F%5C:chrome_wpt_tests%21webtest::external%2Fwpt%2Fhtml%2Fdom%23idlharness.https.html%3Finclude=HTML.+?q=VHash%3A6129bda16b001a29)
A revert for this change was not created because the builder that this CL broke is not watched by gardeners, therefore less important. You can consider revert this CL, fix forward or let builder owners resolve it themselves.
If this is a false positive, please report it at http://b.corp.google.com/createIssue?component=1199205&description=Analysis%3A+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Ftest-analysis%2Fb%2F5205535692423168&format=PLAIN&priority=P3&title=Wrongly+blamed+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F7899676&type=BUG
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |