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.
Created attachment 234744 [details] Testcase
Spec: html.spec.whatwg.org/#selector-enabled
Created attachment 235379 [details] Patch
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.
Created attachment 235429 [details] Patch
Created attachment 235431 [details] Rebased patch
Comment on attachment 235431 [details] Rebased patch Clearing flags on attachment: 235431 Committed r171671: <http://trac.webkit.org/changeset/171671>
All reviewed patches have been landed. Closing bug.
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
Created attachment 238724 [details] Patch
Created attachment 238734 [details] Patch (w/ updated code for Anchor element)
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.
(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 ;)
Created attachment 238855 [details] Patch
Created attachment 238857 [details] Patch
Comment on attachment 238857 [details] Patch Clearing flags on attachment: 238857 Committed r174064: <http://trac.webkit.org/changeset/174064>
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?
(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.
(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.