Commit 3505b982 authored by Yu Han's avatar Yu Han Committed by Commit Bot

Reland "Fixes click on inline element embedded in anchor inside <summary> breaks anchor."

This reverts commit b4cb14f3.

Reason for revert: Getting ready to re-land

The original fix for Bug: 538283 introduced a regression, Bug: 1045433.
So it was reverted. This is another fix for the original issue and
 ensures anchor behavior does not regressed.

To recap, the original issue is caused by DOMActivate event generated
from inline elements. DOMActivate bypasses anchor, bubbles up to
<summary>, and gets handled. The underlying event, click, stops
propagating and terminates. The original fix for anchor looks at the
underlying event and handles it if it's click. However, this logic is
wrong because the anchor can preemptively handle a click that
belongs to another element earlier in the event path.

This example, in file label-inside-anchor.html, illustrate the problem.

<a href="javascript:void(0)" target="_blank">
    <label for="peas"><span id="text">peas?</span></label>
    <input type="checkbox" name="peas" id="peas">
</a>

Clicking on 'text' generates DOMActivate, which is handled by <a>.
Prior to my original fix, DOMActivate isn't handled, and click event
bubbles up until it's handle by the <label> attribute. Bug: 1045433 is
another example of the same behavior. Video element wrapped by anchor.

In this fix, summary element uses the Node property
HasActivationBehavior and does not handle DOMActivate if the target
node or any of its ancestors up itself has this property set. This
allows the click event to continue propagating until it gets handled
by an node with ActivationBehavior.


Original change's description:
> Revert "Fixes click on inline element embedded in anchor inside <summary> breaks anchor."
>
> This reverts commit 4eb4df63.
>
> Reason for revert: Caused crbug.com/1045433.
>
> Original change's description:
> > Fixes click on inline element embedded in anchor inside <summary> breaks anchor.
> >
> > Previous to this CL, clicking on an inline element embedded in an anchor placed
> > inside a <summary> tag will expand the <details> section instead of navigating to
> > the anchor's href. However, when the anchor is placed outside of <summary>,
> > it behaves correctly.
> >
> > The error is caused by DOMActivate event generated by the inline element. As
> > DOMActivate bubbles up, it bypasses the anchor's event handler, reaches the
> > <summary>, and is handled there. Once DOMActivate is handled, the original
> > click event stops propagating and terminates. This behavior, however, differs
> > from when the anchor tag is placed outside of the summary. DOMActivate isn't
> > handled, and the original click event keeps bubbling up till it's handled by
> > the anchor.
> >
> > DOMActivate event is deprecated:
> > https://developer.mozilla.org/en-US/docs/Web/API/Element/DOMActivate_event.
> > However, since blink still has code that depends on it, replacing it is outside
> > of the scope for this fix. Instead, this fix is for the anchor element to
> > handle the DOMActivate event as it bubbles up. The anchor event handler checks
> > the underlying event of DOMActivate and handles it if it's a click.
> >
> > I also looked at an alternative fix by trying to prevent the DOMActivate event
> > from bubbling up. But calling event.stopPropagation() doesn't work as the
> > EventDispatcher::DispatchEventPostProcess doesn't check this status.
> >
> >
> > Bug: 538283
> > Change-Id: I11fb072faa0563279d43a28e5dc19cee89906bf0
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1928234
> > Reviewed-by: Kent Tamura <tkent@chromium.org>
> > Reviewed-by: Mason Freed <masonfreed@chromium.org>
> > Commit-Queue: Yu Han <yuzhehan@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#718552}
>>
> Bug: 1045433
> Change-Id: I14b369beb04171ef846cb3a79ebb3fe268cf5c89
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2023267
> Reviewed-by: Mason Freed <masonfreed@chromium.org>
> Commit-Queue: Yu Han <yuzhehan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#735575}

Bug: 1045433
Change-Id: I0cdd7b48a2c4e9bf2b653edef9ce7970d7e64938
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2028455Reviewed-by: default avatarMason Freed <masonfreed@chromium.org>
Commit-Queue: Yu Han <yuzhehan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#740380}
parent 1b67153c
......@@ -84,14 +84,21 @@ bool HTMLSummaryElement::IsMainSummary() const {
return false;
}
static bool IsClickableControl(Node* node) {
bool HTMLSummaryElement::IsClickableControl(Node* node) {
auto* element = DynamicTo<Element>(node);
if (!element)
return false;
if (element->IsFormControlElement())
return true;
Element* host = element->OwnerShadowHost();
return host && host->IsFormControlElement();
if (host && host->IsFormControlElement())
return true;
while (node && this != node) {
if (node->HasActivationBehavior())
return true;
node = node->ParentOrShadowHostNode();
}
return false;
}
bool HTMLSummaryElement::SupportsFocus() const {
......
......@@ -46,6 +46,7 @@ class HTMLSummaryElement final : public HTMLElement {
bool SupportsFocus() const override;
int DefaultTabIndex() const override;
bool IsClickableControl(Node*);
};
} // namespace blink
......
<!DOCTYPE html>
<meta charset="utf-8">
<title>label element: clicking on label containing inline element placed inside &lt;a&gt; </title>
<link rel="author" title="Yu Han" href="mailto:yuzhehan@chromium.org">
<link rel="help" href="https://html.spec.whatwg.org/multipage/forms.html#the-label-element">
<link rel="help" href="https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<a href="javascript:void(0)" target="_blank">
<label for="peas"><span id="text">peas?</span></label>
<input type="checkbox" name="peas" id="peas">
</a>
<script>
const text = document.getElementById('text'),
peas_cb = document.getElementById('peas');
t1 = async_test("click on inline element inside a label that's placed inside a anchor should trigger default label behavior");
peas_cb.onchange = t1.step_func_done(function(e) {
assert_true(peas_cb.checked, "checkbox is checked");
});
t1.step(function() {
text.click();
});
</script>
\ No newline at end of file
<!DOCTYPE html>
<meta charset="utf-8">
<title>summary element: clicking on anchor containing inline element</title>
<link rel="author" title="Yu Han" href="mailto:yuzhehan@chromium.org">
<link rel="help" href="https://html.spec.whatwg.org/C/#the-summary-element">
<link rel="help" href="https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<details id="details_i">
<summary>Anchor text is wrapped with &lt;i&gt; tag <a href="#with_i_tag"><i id="with_i">permalink</i></a></summary>
<p>asdf</p>
</details>
<details id="details_span">
<summary>This one uses &lt;span&gt;. <a href="#with_span_tag"><span id="with_span">permalink</span></a></summary>
<p>asdf</p>
</details>
<details id="details_svg">
<summary>
<svg style="width: 100px;" viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg">
<a href="#inside_svg_w_circle">
<circle id="svg_circle" cx="50" cy="40" r="35"/>
</a>
<a href="#inside_svg_w_text">
<text id="svg_text" x="50" y="90" text-anchor="middle">
&lt;circle&gt;
</text>
</a>
</svg>
</summary>
<p>asdf</p>
</details>
<script>
function testClickingOnInlineElement(detailsId, targetId, expected, testName) {
const details = document.getElementById(detailsId);
const target = document.getElementById(targetId);
const test = async_test(testName);
const promise = new Promise((resolve, reject) => {
window.onhashchange = test.step_func_done(() => {
assert_false(details.open);
assert_true(location.hash === expected);
resolve();
});
});
if (target.click) {
target.click();
}
else {
// svg element don't have click method
target.dispatchEvent(new MouseEvent('click', {
view: window,
bubbles: true,
cancelable: true
}));
}
return promise;
};
async function testAll() {
try {
await testClickingOnInlineElement("details_i", "with_i", "#with_i_tag", "Expected <a> containing <i> to navigate");
await testClickingOnInlineElement("details_span", "with_span", "#with_span_tag", "Expected <a> containing <span> to navigate");
await testClickingOnInlineElement("details_svg", "svg_circle", "#inside_svg_w_circle", "Expected <a>, inside svg, containing <circle> to navigate");
await testClickingOnInlineElement("details_svg", "svg_text", "#inside_svg_w_text", "Expected <a>, inside svg, containing <text> to navigate");
} catch (exception) {
assert_unreached("should NOT-THROW exception");
}
};
var allTests = async_test("Clicking on anchor with embedded inline element should navigate instead of opening details");
testAll().then(()=>{ allTests.done(); });
</script>
\ No newline at end of file
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment