HTMLLinkElement.disabled does not forward value to the Stylesheet's disabled attribute on setting

• Component: CSS
• Status: RESOLVED INVALID

[Bijan Amirzada]

Created attachment 66970 [details]
Patch

+++ This bug was initially created as a clone of Bug #25287 +++

See testcase: attached

Per the html5 spec 4.2.7 - The disabled IDL attribute on link and style elements must return false and do nothing on setting, if the sheet attribute of their LinkStyle interface is null. Otherwise, it must return the value of the StyleSheet interface's disabled attribute on getting, and forward the new value to that same attribute on setting.

Currently, the HTMLLinkElement.disabled is not forwarding the new value to the Stylesheet's disabled attribute on setting.

[Bijan Amirzada]

Created attachment 67210 [details]
Updated Patch

[WebKit Review Bot]

Attachment 67210 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/HTMLLinkElement.cpp:393:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Bijan Amirzada]

Created attachment 67721 [details]
Updated Patch - per style bot mod request

[Bijan Amirzada]

Comment on attachment 67210 [details]
Updated Patch

This patch is now obsolete.

[Darin Adler]

Comment on attachment 67721 [details]
Updated Patch - per style bot mod request

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

Looks good. Nice test case. Some small stylistic problems that we should fix.

> LayoutTests/fast/html/htmllink-disable-expected.txt:1
> +This test checks HTMLLinkElement disabled conforms to HTML5 spec for the bug id #45425 .

A slightly better directory for this test would be:

    fast/dom/HTMLLinkElement

You’ll find other tests of HTMLLinkElement DOM API there.

> WebCore/html/HTMLLinkElement.cpp:397
> +    if (StyleSheet* styleSheet = this->sheet())
> +        return styleSheet->disabled();
> +
> +    return false;

In WebKit we normally use the early return style rather than nesting code. Also, the "this->" notation would only be needed if the local variable was named the same as the data member. There’s also no reason to use a non-inline function to get one of our own data members. This function should just say:

    return m_sheet && m_sheet->disabled();

> WebCore/html/HTMLLinkElement.cpp:408
> +    if (StyleSheet* styleSheet = this->sheet())
> +        styleSheet->setDisabled(setDisabled);
> +}
> +
> +
> +
>  }

It doesn’t make sense to name the argument “set disabled”. You could name it “disabled” or “flag” or something like that.

This function should just be:

    if (!m_sheet)
        return;
    m_sheet->setDisabled(disabled);

Just one blank line after the function, please, not three.

> WebCore/html/HTMLLinkElement.h:76
> +    bool disabled();

This could and should be a const member function.

[Alexey Proskuryakov]

Created attachment 82832 [details]
with style fixes

I tried to update this patch for landing, but it actually breaks a regression test, fast/dom/boolean-attribute-reflection.html.

[Julien Chaffraix]

Created attachment 88904 [details]
Upated the patch on top of ToT - removed testing for boolean reflected attribute on link:disabled per HTML5 standard & other browsers' behavior

[WebKit Commit Bot]

Comment on attachment 88904 [details]
Upated the patch on top of ToT - removed testing for boolean reflected attribute on link:disabled per HTML5 standard & other browsers' behavior

Clearing flags on attachment: 88904

Committed r84329: <http://trac.webkit.org/changeset/84329>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This appears to have caused https://bugs.webkit.org/show_bug.cgi?id=61400

[Alexey Proskuryakov]

This bug is still RESOLVED/FIXED, but it looks like the changes have been reverted in r94369. Should it be reopened?

[Alexey Proskuryakov]

If we want to track this as bug 67436 now, maybe this bug should be changed to INVALID?

[Julien Chaffraix]

(In reply to comment #12)
> If we want to track this as bug 67436 now, maybe this bug should be changed to INVALID?

That's the idea. Sorry for putting the wrong state.