[mathml] Initial Support for MathMLAnchorElement under MathML namespace [chromium/src : main]
tannal (Gerrit)
tannal added 4 comments![]( "Open in Gerrit")
math a:link,TODO: add a comment here, to explain all three css rule.
return {ElementType::kMathMLElement};Maybe change this line to `{element.GetElementType()}`
event.SetDefaultHandled();Add tests converge for most of the method of `MathMLAnchorElement` in the follow up CLs.
interface MathMLAnchorElement : MathMLElement {TODO: Add a comment about adding those idl attributes and testing them in the follow up CLs.
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. |
Stephen Chenney (Gerrit)
Stephen Chenney added 2 comments![]( "Open in Gerrit")
I'm guessing much of this is very similar to other link elements. Is that true?
void MathMLAnchorElement::HandleClick(MouseEvent& event) {How much of this is the same as that for the HTML <a> element? It seems like it should be very similar and if so the code should somehow be extracted out into a utility class to be used by all link elements.
I recognise that this might depend on the other work to re-arrange the IDL hierarchy for the various link elements.
Related details
- Frédéric Wang Nélar
- 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")
void MathMLAnchorElement::HandleClick(MouseEvent& event) {How much of this is the same as that for the HTML <a> element? It seems like it should be very similar and if so the code should somehow be extracted out into a utility class to be used by all link elements.
I recognise that this might depend on the other work to re-arrange the IDL hierarchy for the various link elements.
Yes, it's similar to svg and html, but not the same.
We already have AnchorElementUtils which is shared by html, svg, and mathml.
It should be fine in this CL I think.
For the IDL hierarchy re-arranging:
The plan is supporting target, download, ping, rel, relList, and referrerPolicy by moving those attributes into HyperlinkElementUtils.[1]
AnchorElementUtils supports all those attributes except target and href.
[1] See https://github.com/w3c/mathml-core/issues/325#issue-4382884390
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. |
tannal (Gerrit)
tannal added 1 comment![]( "Open in Gerrit")
if (params.name == html_names::kHrefAttr) {Should we change `html_names` to `mathml_names` for all the code in this file?
I'm not sure what's the difference.
tannal (Gerrit)
tannal added 2 comments![]( "Open in Gerrit")
TODO: add a comment here, to explain all three css rule.
Done
AnchorElementUtils::HandleReferrerPolicyAttribute(Should we remove the unsupported attributes for now?
Is it ok to keep them here?
Since IDL file for MathMLAnchorElement is empty for now.
E.G. download, ping, rel, relList, and referrerPolicy attributes.
Related details
- Fredrik Söderquist
- 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. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 6 comments![]( "Open in Gerrit")
You probably also want to hook up the element in:
* `ContextMenuController::ShowContextMenu` (for `download`)
* `WebLocalFrameImpl::SendPings` (for `ping`)
...and maybe more.
return mathml_a->Url();I think you can just use the same code-path as for `<html:a>` instead. (`<svg:a>` has a special-case here because it has a little different attribute setup, so it's not the case to copy.)
bool IsEnterKeyKeydownEvent(Event& event);
bool IsLinkClick(Event& event);Just include `html_anchor_element.h` for these. It's a bit ugly, and maybe they should move to some shared location (`IsLinkClick` could probably move to `anchor_element_utils.*`, `IsEnterKeyKeydownEvent` has less obvious affinity, but maybe next to `KeyboardEvent`?).
if (params.name == html_names::kHrefAttr) {Should we change `html_names` to `mathml_names` for all the code in this file?
I'm not sure what's the difference.
Fredrik Söderquist
They will be the same for all practical purposes (both have the same localname and are in the null NS). It might be that IDL `[Reflect]` will require the latter though (that's the case for SVG at least).
return GetDocument().CompleteURL(StripLeadingAndTrailingHtmlSpaces(
FastGetAttribute(html_names::kHrefAttr)));You probably want to align with `HTMLAnchorElement(Base)` here. Otherwise this is `GetURLAttributeAsKURL(html_names::kHrefAttr)`.
AnchorElementUtils::HandleReferrerPolicyAttribute(Should we remove the unsupported attributes for now?
Is it ok to keep them here?
Since IDL file for MathMLAnchorElement is empty for now.E.G. download, ping, rel, relList, and referrerPolicy attributes.
If you intend to add them (I assume so?) it seems fine to keep them. We should just have all shared code in a function called by all "anchors" (we should be close to that point).
Related details
- Frédéric Wang Nélar
- Stephen Chenney
- tannal
tannal (Gerrit)
tannal added 4 comments![]( "Open in Gerrit")
I think you can just use the same code-path as for `<html:a>` instead. (`<svg:a>` has a special-case here because it has a little different attribute setup, so it's not the case to copy.)
Done
bool IsEnterKeyKeydownEvent(Event& event);
bool IsLinkClick(Event& event);Just include `html_anchor_element.h` for these. It's a bit ugly, and maybe they should move to some shared location (`IsLinkClick` could probably move to `anchor_element_utils.*`, `IsEnterKeyKeydownEvent` has less obvious affinity, but maybe next to `KeyboardEvent`?).
I agree that it's ugly to include `html_anchor_element.h` for svg and mathml.
I moved these methods to `anchor_element_utils` and `KeyboardEvent` as a static method.
return GetDocument().CompleteURL(StripLeadingAndTrailingHtmlSpaces(
FastGetAttribute(html_names::kHrefAttr)));You probably want to align with `HTMLAnchorElement(Base)` here. Otherwise this is `GetURLAttributeAsKURL(html_names::kHrefAttr)`.
`HTMLAnchorElement(Base)` use the same code for `Url` mehthod.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_anchor_element.cc;l=344?q=third_party%2Fblink%2Frenderer%2Fcore%2Fhtml%2Fhtml_anchor_element.cc
AnchorElementUtils::HandleReferrerPolicyAttribute(Fredrik SöderquistShould we remove the unsupported attributes for now?
Is it ok to keep them here?
Since IDL file for MathMLAnchorElement is empty for now.E.G. download, ping, rel, relList, and referrerPolicy attributes.
If you intend to add them (I assume so?) it seems fine to keep them. We should just have all shared code in a function called by all "anchors" (we should be close to that point).
Yeah, I'll add those attributes in the follow up CLs.
Do you know where the wpt tests of those attributes are?
They are not easy to find.
Related details
- Fredrik Söderquist
- 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")
a:link,So is a:link needed? isn't it implied by a:-webkit-any-link?
return 0;nit: maybe add a link to the HTML spec where it says the default tabindex for the mathml a element is 0?
Related details
- Fredrik Söderquist
- 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 2 comments![]( "Open in Gerrit")
So is a:link needed? isn't it implied by a:-webkit-any-link?
It's not needed. Removed.
nit: maybe add a link to the HTML spec where it says the default tabindex for the mathml a element is 0?
- Fredrik Söderquist
- 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. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 3 comments![]( "Open in Gerrit")
bool IsEnterKeyKeydownEvent(Event& event);
bool IsLinkClick(Event& event);tannalJust include `html_anchor_element.h` for these. It's a bit ugly, and maybe they should move to some shared location (`IsLinkClick` could probably move to `anchor_element_utils.*`, `IsEnterKeyKeydownEvent` has less obvious affinity, but maybe next to `KeyboardEvent`?).
I agree that it's ugly to include `html_anchor_element.h` for svg and mathml.
I moved these methods to `anchor_element_utils` and `KeyboardEvent` as a static method.
Acknowledged
return GetDocument().CompleteURL(StripLeadingAndTrailingHtmlSpaces(
FastGetAttribute(html_names::kHrefAttr)));tannalYou probably want to align with `HTMLAnchorElement(Base)` here. Otherwise this is `GetURLAttributeAsKURL(html_names::kHrefAttr)`.
`HTMLAnchorElement(Base)` use the same code for `Url` mehthod.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_anchor_element.cc;l=344?q=third_party%2Fblink%2Frenderer%2Fcore%2Fhtml%2Fhtml_anchor_element.cc
No...?
```
KURL HTMLAnchorElementBase::Href() const {
return GetDocument().CompleteURL(StripLeadingAndTrailingHtmlSpaces(
FastGetAttribute(html_names::kHrefAttr)));
}
...
KURL HTMLAnchorElementBase::Url() const {
KURL href = Href();
if (!href.IsValid()) {
return KURL();
}
return href;
}
```
`Href()` is the same, but `Url()` is different.
AnchorElementUtils::HandleReferrerPolicyAttribute(Fredrik SöderquistShould we remove the unsupported attributes for now?
Is it ok to keep them here?
Since IDL file for MathMLAnchorElement is empty for now.E.G. download, ping, rel, relList, and referrerPolicy attributes.
tannalIf you intend to add them (I assume so?) it seems fine to keep them. We should just have all shared code in a function called by all "anchors" (we should be close to that point).
Yeah, I'll add those attributes in the follow up CLs.
Do you know where the wpt tests of those attributes are?
They are not easy to find.
Some SVG ones should be in `svg/linking/scripted`, for HTML some are in `html/semantics/links` (and there are most likely more depending on the facet).
Related details
- Frédéric Wang Nélar
- Stephen Chenney
- tannal
tannal (Gerrit)
tannal added 3 comments![]( "Open in Gerrit")
if (params.name == html_names::kHrefAttr) {Fredrik SöderquistShould we change `html_names` to `mathml_names` for all the code in this file?
I'm not sure what's the difference.
They will be the same for all practical purposes (both have the same localname and are in the null NS). It might be that IDL `[Reflect]` will require the latter though (that's the case for SVG at least).
Acknowledged
return GetDocument().CompleteURL(StripLeadingAndTrailingHtmlSpaces(
FastGetAttribute(html_names::kHrefAttr)));tannalYou probably want to align with `HTMLAnchorElement(Base)` here. Otherwise this is `GetURLAttributeAsKURL(html_names::kHrefAttr)`.
Fredrik Söderquist`HTMLAnchorElement(Base)` use the same code for `Url` mehthod.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_anchor_element.cc;l=344?q=third_party%2Fblink%2Frenderer%2Fcore%2Fhtml%2Fhtml_anchor_element.cc
No...?
```
KURL HTMLAnchorElementBase::Href() const {
return GetDocument().CompleteURL(StripLeadingAndTrailingHtmlSpaces(
FastGetAttribute(html_names::kHrefAttr)));
}
...
KURL HTMLAnchorElementBase::Url() const {
KURL href = Href();
if (!href.IsValid()) {
return KURL();
}
return href;
}
```
`Href()` is the same, but `Url()` is different.
So the difference is the check?
```
if (!href.IsValid()) {
return KURL();
}
```
I'll add that later.
AnchorElementUtils::HandleReferrerPolicyAttribute(Fredrik SöderquistShould we remove the unsupported attributes for now?
Is it ok to keep them here?
Since IDL file for MathMLAnchorElement is empty for now.E.G. download, ping, rel, relList, and referrerPolicy attributes.
tannalIf you intend to add them (I assume so?) it seems fine to keep them. We should just have all shared code in a function called by all "anchors" (we should be close to that point).
Fredrik SöderquistYeah, I'll add those attributes in the follow up CLs.
Do you know where the wpt tests of those attributes are?
They are not easy to find.
Some SVG ones should be in `svg/linking/scripted`, for HTML some are in `html/semantics/links` (and there are most likely more depending on the facet).
- Fredrik Söderquist
- Frédéric Wang Nélar
- Stephen Chenney
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
return GetDocument().CompleteURL(StripLeadingAndTrailingHtmlSpaces(
FastGetAttribute(html_names::kHrefAttr)));tannalYou probably want to align with `HTMLAnchorElement(Base)` here. Otherwise this is `GetURLAttributeAsKURL(html_names::kHrefAttr)`.
Fredrik Söderquist`HTMLAnchorElement(Base)` use the same code for `Url` mehthod.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_anchor_element.cc;l=344?q=third_party%2Fblink%2Frenderer%2Fcore%2Fhtml%2Fhtml_anchor_element.cc
tannalNo...?
```
KURL HTMLAnchorElementBase::Href() const {
return GetDocument().CompleteURL(StripLeadingAndTrailingHtmlSpaces(
FastGetAttribute(html_names::kHrefAttr)));
}
...
KURL HTMLAnchorElementBase::Url() const {
KURL href = Href();
if (!href.IsValid()) {
return KURL();
}
return href;
}
```
`Href()` is the same, but `Url()` is different.
So the difference is the check?
```
if (!href.IsValid()) {
return KURL();
}
```
I'll add that later.
So the difference is the check?
- Frédéric Wang Nélar
- Stephen Chenney
- tannal
tannal (Gerrit)
tannal added 2 comments![]( "Open in Gerrit")
return GetDocument().CompleteURL(StripLeadingAndTrailingHtmlSpaces(
FastGetAttribute(html_names::kHrefAttr)));tannalYou probably want to align with `HTMLAnchorElement(Base)` here. Otherwise this is `GetURLAttributeAsKURL(html_names::kHrefAttr)`.
Fredrik Söderquist`HTMLAnchorElement(Base)` use the same code for `Url` mehthod.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_anchor_element.cc;l=344?q=third_party%2Fblink%2Frenderer%2Fcore%2Fhtml%2Fhtml_anchor_element.cc
tannalNo...?
```
KURL HTMLAnchorElementBase::Href() const {
return GetDocument().CompleteURL(StripLeadingAndTrailingHtmlSpaces(
FastGetAttribute(html_names::kHrefAttr)));
}
...
KURL HTMLAnchorElementBase::Url() const {
KURL href = Href();
if (!href.IsValid()) {
return KURL();
}
return href;
}
```
`Href()` is the same, but `Url()` is different.
Fredrik SöderquistSo the difference is the check?
```
if (!href.IsValid()) {
return KURL();
}
```
I'll add that later.
So the difference is the check?
Yes. I assume it matters for `Origin`.
Thanks for the feedback.
Done.
void MathMLAnchorElement::HandleClick(MouseEvent& event) {tannalHow much of this is the same as that for the HTML <a> element? It seems like it should be very similar and if so the code should somehow be extracted out into a utility class to be used by all link elements.
I recognise that this might depend on the other work to re-arrange the IDL hierarchy for the various link elements.
Yes, it's similar to svg and html, but not the same.
We already have AnchorElementUtils which is shared by html, svg, and mathml.
It should be fine in this CL I think.
For the IDL hierarchy re-arranging:
The plan is supporting target, download, ping, rel, relList, and referrerPolicy by moving those attributes into HyperlinkElementUtils.[1]
AnchorElementUtils supports all those attributes except target and href.
[1] See https://github.com/w3c/mathml-core/issues/325#issue-4382884390
- Fredrik Söderquist
- Frédéric Wang Nélar
- Stephen Chenney
Frédéric Wang Nélar (Gerrit)
Frédéric Wang Nélar added 1 comment![]( "Open in Gerrit")
- Fredrik Söderquist
- 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 5 comments![]( "Open in Gerrit")
I'm done with the code change.
I think I addressed all the feedbacks.
Just resolved my todo comments and make sure I'm not blocking any reviever.
tannalSo can you please link to the intent to prototype now?
Done
Maybe change this line to `{element.GetElementType()}`
I think this can be done in the follow up CLs.
event.SetDefaultHandled();tannalAdd tests converge for most of the method of `MathMLAnchorElement` in the follow up CLs.
I think this can be done in the follow up CLs.
interface MathMLAnchorElement : MathMLElement {tannalTODO: Add a comment about adding those idl attributes and testing them in the follow up CLs.
I think this can be done in the follow up CLs.
Related details
- Fredrik Söderquist
- Frédéric Wang Nélar
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist voted Code-Review+1![]( "Open in Gerrit")
- Frédéric Wang Nélar
- 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. |
tannal (Gerrit)
tannal added 1 comment![]( "Open in Gerrit")
- Fredrik Söderquist
- Frédéric Wang Nélar
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist voted Code-Review+1![]( "Open in Gerrit")
- Frédéric Wang Nélar
- Stephen Chenney
- tannal
Frédéric Wang Nélar (Gerrit)
Frédéric Wang Nélar voted![]( "Open in Gerrit")
- 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. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[mathml] Initial Support for MathMLAnchorElement under MathML namespace
Introduce the MathMLAnchorElement behind a new runtime feature flag
'MathMLAnchorElement'. When enabled, this creates the standard <a>
element within the MathML namespace.
See https://w3c.github.io/mathml-core/#the-a-element
Demos: https://people.igalia.com/fwang/mathml-a-href/
I2P: https://groups.google.com/a/chromium.org/g/blink-dev/c/kmtYoVZjtZE
- M third_party/blink/renderer/bindings/generated_in_core.gni
- M third_party/blink/renderer/bindings/idl_in_core.gni
- M third_party/blink/renderer/core/css/mathml.css
- M third_party/blink/renderer/core/css/resolver/style_adjuster.cc
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/core/dom/visited_link_state.cc
- M third_party/blink/renderer/core/events/keyboard_event.cc
- M third_party/blink/renderer/core/events/keyboard_event.h
- M third_party/blink/renderer/core/html/anchor_element_utils.cc
- M third_party/blink/renderer/core/html/anchor_element_utils.h
- M third_party/blink/renderer/core/html/html_anchor_element.cc
- M third_party/blink/renderer/core/html/html_anchor_element.h
- M third_party/blink/renderer/core/mathml/build.gni
- A third_party/blink/renderer/core/mathml/mathml_anchor_element.cc
- A third_party/blink/renderer/core/mathml/mathml_anchor_element.h
- A third_party/blink/renderer/core/mathml/mathml_anchor_element.idl
- M third_party/blink/renderer/core/mathml/mathml_tag_names.json5
- M third_party/blink/renderer/core/svg/svg_a_element.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/TestExpectations
- D third_party/blink/web_tests/external/wpt/mathml/relations/html5-tree/dynamic-href-002-expected.txt
- D third_party/blink/web_tests/external/wpt/mathml/relations/html5-tree/href-click-003-expected.txt
- D third_party/blink/web_tests/external/wpt/mathml/relations/html5-tree/html-or-svg-or-mathml-element-interfaces-expected.txt
- D third_party/blink/web_tests/external/wpt/mathml/relations/html5-tree/tabindex-001-expected.txt
- D third_party/blink/web_tests/external/wpt/mathml/relations/html5-tree/tabindex-focus-001-expected.txt
Code-Review: +1 by Frédéric Wang Nélar, +1 by Fredrik Söderquist
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |