Bug 134826 – Anchor, Area and Link elements should *not* respond to :enabled selector if they have an href attribute

• Component: DOM
• Status: RESOLVED FIXED

[Bruno Abinader]

What steps will reproduce the problem?
1. Open attached testcase 

What is the expected output? What do you see instead?
Query for :enabled should be non-null.

[Bruno Abinader]

Created attachment 234744 [details]
Testcase

[Bruno Abinader]

Spec:
html.spec.whatwg.org/#selector-enabled

[Bruno Abinader]

Created attachment 235379 [details]
Patch

[Darin Adler]

Comment on attachment 235379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235379&action=review

Patch doesn’t apply.

> Source/WebCore/css/SelectorCheckerTestFunctions.h:67
> +    else if (isHTMLAnchorElement(element) || isHTMLAreaElement(element))
> +        return element->isLink();

Minor style nit: We don’t do else after return.

Performance nit: Should check the isLink bit first, before looking at the the element type, for better performance.

[Bruno Abinader]

Created attachment 235429 [details]
Patch

[Bruno Abinader]

Created attachment 235431 [details]
Rebased patch

[WebKit Commit Bot]

Comment on attachment 235431 [details]
Rebased patch

Clearing flags on attachment: 235431

Committed r171671: <http://trac.webkit.org/changeset/171671>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Bruno Abinader]

Anchors, Areas & Links are no longer matched by :enabled CSS selector [1], according to this discussion from W3C Bugzilla [2]. Thus, I'm going to revert these changes.

Links:
[1] https://html5.org/r/8818
[2] https://www.w3.org/Bugs/Public/show_bug.cgi?id=26622

[Bruno Abinader]

Created attachment 238724 [details]
Patch

[Bruno Abinader]

Created attachment 238734 [details]
Patch (w/ updated code for Anchor element)

[Darin Adler]

Comment on attachment 238734 [details]
Patch (w/ updated code for Anchor element)

View in context: https://bugs.webkit.org/attachment.cgi?id=238734&action=review

Hope nobody shipped a browser between when we changed this and reverted the change!

> Source/WebCore/css/SelectorCheckerTestFunctions.h:68
> -    bool isEnabled = false;
>      if (is<HTMLFormControlElement>(element) || is<HTMLOptionElement>(element) || is<HTMLOptGroupElement>(element))
> -        isEnabled = !element->isDisabledFormControl();
> -    else if (element->isLink())
> -        isEnabled = is<HTMLAnchorElement>(element) || is<HTMLAreaElement>(element);
> -    return isEnabled;
> +        return !element->isDisabledFormControl();
> +    return false;

This now seems like it would be clearer with && than with if.

[Bruno Abinader]

(In reply to comment #12)
> (From update of attachment 238734 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238734&action=review
> 
> Hope nobody shipped a browser between when we changed this and reverted the change!
> 
> > Source/WebCore/css/SelectorCheckerTestFunctions.h:68
> > -    bool isEnabled = false;
> >      if (is<HTMLFormControlElement>(element) || is<HTMLOptionElement>(element) || is<HTMLOptGroupElement>(element))
> > -        isEnabled = !element->isDisabledFormControl();
> > -    else if (element->isLink())
> > -        isEnabled = is<HTMLAnchorElement>(element) || is<HTMLAreaElement>(element);
> > -    return isEnabled;
> > +        return !element->isDisabledFormControl();
> > +    return false;
> 
> This now seems like it would be clearer with && than with if.

Indeed, the odds of live specifications, I guess ;)

[Bruno Abinader]

Created attachment 238855 [details]
Patch

[Bruno Abinader]

Created attachment 238857 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 238857 [details]
Patch

Clearing flags on attachment: 238857

Committed r174064: <http://trac.webkit.org/changeset/174064>

[Benjamin Poulain]

Instead of removing the tests, you should have modified css-selector-enabled-links.html to ensure that :enabled *do not* match links.

Can you please create a new patch to test this properly?

[Bruno Abinader]

(In reply to comment #17)
> Instead of removing the tests, you should have modified css-selector-enabled-links.html to ensure that :enabled *do not* match links.
> 
> Can you please create a new patch to test this properly?

Sure, I'll re-add the tests with updated results.

[Benjamin Poulain]

(In reply to comment #18)
> (In reply to comment #17)
> > Instead of removing the tests, you should have modified css-selector-enabled-links.html to ensure that :enabled *do not* match links.
> > 
> > Can you please create a new patch to test this properly?
> 
> Sure, I'll re-add the tests with updated results.

Thanks. Please CC me on the bug.