Summary: | Anchor, Area and Link elements should *not* respond to :enabled selector if they have an href attribute | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bruno Abinader <brunoabinader> | ||||||||||||||||||
Component: | DOM | Assignee: | Bruno Abinader <brunoabinader> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, darin | ||||||||||||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 137557 | ||||||||||||||||||||
Attachments: |
|
Description
Bruno Abinader
2014-07-10 21:50:52 PDT
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. |