Changeset 93425 in webkit


Ignore:
Timestamp:
Aug 19, 2011 11:54:08 AM (13 years ago)
Author:
jchaffraix@webkit.org
Message:

REGRESSION (r84327-r84329): CSS stylesheets fail to load on www.flagstar.com login page
​https://bugs.webkit.org/show_bug.cgi?id=65140
<rdar://problem/9835905>

Reviewed by Antti Koivisto.

Source/WebCore:

Tests: fast/css/stylesheet-enable-first-alternate-on-load.html

fast/css/stylesheet-enable-first-alternate.html
fast/css/stylesheet-enable-second-alternate-on-load.html
fast/css/stylesheet-enable-second-alternate.html
http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error.html
http/tests/css/link-css-disabled-value-with-slow-loading-sheet.html

The gist of the issue is that we were ignoring calls to HTMLLinkElement::setDisabled that would enable a
style sheet when we were loading a stylesheet (m_sheet was 0 and thus ignored the call per the spec).

FF goes against the CSS OM spec in this case and always keep an associated sheet as long as 'rel' hints
at a stylesheet link and href is present. Instead of siding with FF, I continued to follow the
specification and store the enabled via javascript state into m_scriptState (renamed from
m_isEnabledViaScript). This information gets merged back into the style sheet disabled state when it is
available.

While debugging the case at hand, I found some cases that were not properly handled and were fixed as
part of this change.

  • html/HTMLLinkElement.cpp:

(WebCore::HTMLLinkElement::HTMLLinkElement): Updated after m_isEnabledViaScript rename.
(WebCore::HTMLLinkElement::setDisabled): Always call setIsEnabledViaScript so that
the information is properly stored (either for recalcStyleSelector or just to store
the state during loading).

(WebCore::HTMLLinkElement::sheetLoaded): Merge back the state from m_scriptState to
the sheet's disabled state.

(WebCore::HTMLLinkElement::disabled): Account for the temporary state and return the
right value. It matches FF and what people would expect.

(WebCore::HTMLLinkElement::areDisabledAndScriptStatesConsistent): Debug only method
that checks that disabled() and isEnabledViaScript() are consistent with each other
(under some circumstances).

  • html/HTMLLinkElement.h:

(WebCore::HTMLLinkElement::isEnabledViaScript): Updated after m_isEnabledViaScript rename.
(WebCore::HTMLLinkElement::setIsEnabledViaScript): Added setter.

LayoutTests:

  • fast/css/link-disabled-attr-expected.txt: This change is a progression, but we are still

not handling updates to the selected style set properly (already covered by bug 62407).

  • fast/css/stylesheet-enable-first-alternate-expected.txt: Added.
  • fast/css/stylesheet-enable-first-alternate-on-load-expected.txt: Added.
  • fast/css/stylesheet-enable-first-alternate-on-load.html: Added.
  • fast/css/stylesheet-enable-first-alternate.html: Added.
  • fast/css/stylesheet-enable-second-alternate-expected.txt: Added.
  • fast/css/stylesheet-enable-second-alternate-on-load-expected.txt: Added.
  • fast/css/stylesheet-enable-second-alternate-on-load.html: Added.
  • fast/css/stylesheet-enable-second-alternate.html: Added.

Those are a variation on the same theme: we disable some stylesheet and enable others,
either directly or later.

  • http/tests/css/link-css-disabled-value-with-slow-loading-sheet-expected.txt: Added.
  • http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error-expected.txt: Added.
  • http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error.html: Added.
  • http/tests/css/link-css-disabled-value-with-slow-loading-sheet.html: Added.
  • http/tests/css/resources/link-css-disabled-value-with-slow-loading-sheet-in-error.js: Added.

(onSheetLoaded):
(testWhenLoaded):

  • http/tests/css/resources/link-css-disabled-value-with-slow-loading-sheet.js: Added.

(onSheetLoaded):
(testWhenLoaded):

  • http/tests/css/resources/slow-loading-sheet-in-error.php: Added.
  • http/tests/css/resources/slow-loading-sheet.php: Added.

Those 2 test cases relies on a slow loading stylesheet so that we can test the disabled() value while the alternate
sheet is asynchronously loading and then check them again once the sheet is loaded.

Location:
trunk
Files:
16 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r93423 r93425  
     12011-08-19  Julien Chaffraix  <jchaffraix@webkit.org>
     2
     3        REGRESSION (r84327-r84329): CSS stylesheets fail to load on www.flagstar.com login page
     4        https://bugs.webkit.org/show_bug.cgi?id=65140
     5        <rdar://problem/9835905>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        * fast/css/link-disabled-attr-expected.txt: This change is a progression, but we are still
     10        not handling updates to the selected style set properly (already covered by bug 62407).
     11
     12        * fast/css/stylesheet-enable-first-alternate-expected.txt: Added.
     13        * fast/css/stylesheet-enable-first-alternate-on-load-expected.txt: Added.
     14        * fast/css/stylesheet-enable-first-alternate-on-load.html: Added.
     15        * fast/css/stylesheet-enable-first-alternate.html: Added.
     16        * fast/css/stylesheet-enable-second-alternate-expected.txt: Added.
     17        * fast/css/stylesheet-enable-second-alternate-on-load-expected.txt: Added.
     18        * fast/css/stylesheet-enable-second-alternate-on-load.html: Added.
     19        * fast/css/stylesheet-enable-second-alternate.html: Added.
     20        Those are a variation on the same theme: we disable some stylesheet and enable others,
     21        either directly or later.
     22
     23        * http/tests/css/link-css-disabled-value-with-slow-loading-sheet-expected.txt: Added.
     24        * http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error-expected.txt: Added.
     25        * http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error.html: Added.
     26        * http/tests/css/link-css-disabled-value-with-slow-loading-sheet.html: Added.
     27        * http/tests/css/resources/link-css-disabled-value-with-slow-loading-sheet-in-error.js: Added.
     28        (onSheetLoaded):
     29        (testWhenLoaded):
     30        * http/tests/css/resources/link-css-disabled-value-with-slow-loading-sheet.js: Added.
     31        (onSheetLoaded):
     32        (testWhenLoaded):
     33        * http/tests/css/resources/slow-loading-sheet-in-error.php: Added.
     34        * http/tests/css/resources/slow-loading-sheet.php: Added.
     35        Those 2 test cases relies on a slow loading stylesheet so that we can test the disabled() value while the alternate
     36        sheet is asynchronously loading and then check them again once the sheet is loaded.
     37
    1382011-08-19  Vitaly Repeshko  <vitalyr@chromium.org>
    239

  • trunk/LayoutTests/fast/css/link-disabled-attr-expected.txt

    r88479 r93425  
    1616FAIL link.disabled should be true. Was false.
    1717PASS link.sheet is non-null.
    18 FAIL getComputedStyle(console).backgroundColor should be rgb(0, 128, 0). Was rgba(0, 0, 0, 0).
     18PASS getComputedStyle(console).backgroundColor is 'rgb(0, 128, 0)'
    1919FAIL link.disabled should be true. Was false.
    20 PASS getComputedStyle(console).backgroundColor is originalBG
     20FAIL getComputedStyle(console).backgroundColor should be rgba(0, 0, 0, 0). Was rgb(0, 128, 0).
    2121PASS link.disabled is false
    22 FAIL getComputedStyle(console).backgroundColor should be rgb(0, 128, 0). Was rgba(0, 0, 0, 0).
     22PASS getComputedStyle(console).backgroundColor is 'rgb(0, 128, 0)'
    2323PASS getComputedStyle(console).backgroundColor is originalBG
    2424PASS successfullyParsed is true

  • trunk/Source/WebCore/ChangeLog

    r93424 r93425  
     12011-08-19  Julien Chaffraix  <jchaffraix@webkit.org>
     2
     3        REGRESSION (r84327-r84329): CSS stylesheets fail to load on www.flagstar.com login page
     4        https://bugs.webkit.org/show_bug.cgi?id=65140
     5        <rdar://problem/9835905>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Tests: fast/css/stylesheet-enable-first-alternate-on-load.html
     10               fast/css/stylesheet-enable-first-alternate.html
     11               fast/css/stylesheet-enable-second-alternate-on-load.html
     12               fast/css/stylesheet-enable-second-alternate.html
     13               http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error.html
     14               http/tests/css/link-css-disabled-value-with-slow-loading-sheet.html
     15
     16        The gist of the issue is that we were ignoring calls to HTMLLinkElement::setDisabled that would enable a
     17        style sheet when we were loading a stylesheet (m_sheet was 0 and thus ignored the call per the spec).
     18
     19        FF goes against the CSS OM spec in this case and always keep an associated sheet as long as 'rel' hints
     20        at a stylesheet link and href is present. Instead of siding with FF, I continued to follow the
     21        specification and store the enabled via javascript state into m_scriptState (renamed from
     22        m_isEnabledViaScript). This information gets merged back into the style sheet disabled state when it is
     23        available.
     24
     25        While debugging the case at hand, I found some cases that were not properly handled and were fixed as
     26        part of this change.
     27
     28        * html/HTMLLinkElement.cpp:
     29        (WebCore::HTMLLinkElement::HTMLLinkElement): Updated after m_isEnabledViaScript rename.
     30        (WebCore::HTMLLinkElement::setDisabled): Always call setIsEnabledViaScript so that
     31        the information is properly stored (either for recalcStyleSelector or just to store
     32        the state during loading).
     33
     34        (WebCore::HTMLLinkElement::sheetLoaded): Merge back the state from m_scriptState to
     35        the sheet's disabled state.
     36
     37        (WebCore::HTMLLinkElement::disabled): Account for the temporary state and return the
     38        right value. It matches FF and what people would expect.
     39
     40        (WebCore::HTMLLinkElement::areDisabledAndScriptStatesConsistent): Debug only method
     41        that checks that disabled() and isEnabledViaScript() are consistent with each other
     42        (under some circumstances).
     43
     44        * html/HTMLLinkElement.h:
     45        (WebCore::HTMLLinkElement::isEnabledViaScript): Updated after m_isEnabledViaScript rename.
     46        (WebCore::HTMLLinkElement::setIsEnabledViaScript): Added setter.
     47
    1482011-08-17  Adrienne Walker  <enne@google.com>
    249

  • trunk/Source/WebCore/html/HTMLLinkElement.cpp

    r93227 r93425  
    5757    , m_sizes(DOMSettableTokenList::create())
    5858    , m_loading(false)
    59     , m_isEnabledViaScript(false)
     59    , m_scriptState(Unset)
    6060    , m_createdByParser(createdByParser)
    6161    , m_isInShadowTree(false)
    … …  
    8686void HTMLLinkElement::setDisabled(bool disabled)
    8787{
     88    bool wasEnabledViaScript = isEnabledViaScript();
     89    setIsEnabledViaScript(!disabled);
     90
    8891    if (!m_sheet)
    8992        return;
    9093
    9194    bool wasDisabled = m_sheet->disabled();
    92     if (wasDisabled == disabled)
    93         return;
     95    if (wasDisabled == disabled) {
     96        if (wasEnabledViaScript != isEnabledViaScript())
     97            document()->styleSelectorChanged(DeferRecalcStyle);
     98        return;
     99    }
    94100
    95101    m_sheet->setDisabled(disabled);
    96     m_isEnabledViaScript = !disabled;
    97102
    98103    // If we change the disabled state while the sheet is still loading, then we have to
    … …  
    121126    if (!disabled)
    122127        process();
     128
     129    ASSERT(areDisabledAndScriptStatesConsistent());
    123130}
    124131
    … …  
    360367bool HTMLLinkElement::sheetLoaded()
    361368{
     369    // Migrate the disabled information before removePendingSheet is called
     370    // as it will start a recalStyleSelector which needs this information.
     371    if (m_scriptState != Unset) {
     372        ASSERT(!m_loading);
     373        // FIXME: We should ASSERT that it was set for stylesheets only, but
     374        // currently we allow setDisabled to be called regardless of the <link> rel.
     375        setDisabled(m_scriptState == DisabledViaScript);
     376    }
     377
    362378    if (!isLoading()) {
    363379        removePendingSheet();
    364380        return true;
    365381    }
     382
    366383    return false;
    367384}
    … …  
    446463bool HTMLLinkElement::disabled() const
    447464{
    448     return m_sheet && m_sheet->disabled();
     465    ASSERT(areDisabledAndScriptStatesConsistent());
     466
     467    if (!m_sheet) {
     468        // FF disagrees with the CSS OM spec and always has an associated stylesheet for alternate sheet (regardless of whether
     469        // the resource is fetched). As we store the enabled state in m_scriptState while loading, return this information to be
     470        // consistent with FF. sheetLoaded() is called at the end of any transfer (whether it was in error or not) so m_scriptState
     471        // will be transfered back into our stylesheet and the disabled() value should always be consistent.
     472        if (isLoading() && m_scriptState != Unset)
     473            return m_scriptState == DisabledViaScript;
     474
     475        return false;
     476    }
     477
     478    return m_sheet->disabled();
    449479}
    450480
    … …  
    459489}
    460490
     491#ifndef NDEBUG
     492bool HTMLLinkElement::areDisabledAndScriptStatesConsistent() const
     493{
     494    if (!m_relAttribute.m_isStyleSheet)
     495        return true;
     496
     497    // During loading, m_scriptState holds the temporary value for sheet()->disabled()
     498    // so it can have any values (same for sheet()->disabled()).
     499    if (isLoading())
     500        return true;
     501
     502    if (m_scriptState == Unset)
     503        return true;
     504
     505    bool isDisabledViaScript = m_scriptState == DisabledViaScript;
     506    return isDisabledViaScript == sheet()->disabled();
     507}
     508#endif
     509
    461510} // namespace WebCore

  • trunk/Source/WebCore/html/HTMLLinkElement.h

    r91893 r93425  
    5858    // FIXME: This should be renamed isStyleSheetLoading as this is only used for stylesheets.
    5959    bool isLoading() const;
    60     bool isEnabledViaScript() const { return m_isEnabledViaScript; }
     60    bool isEnabledViaScript() const { return m_scriptState == EnabledViaScript; }
    6161    bool disabled() const;
    6262    void setDisabled(bool);
    … …  
    8686    virtual bool isURLAttribute(Attribute*) const;
    8787
    88 private:
    8988    virtual void addSubresourceAttributeURLs(ListHashSet<KURL>&) const;
    9089
    … …  
    9897    HTMLLinkElement(const QualifiedName&, Document*, bool createdByParser);
    9998
     99    void setIsEnabledViaScript(bool enabled) { m_scriptState = enabled ? EnabledViaScript : DisabledViaScript; }
     100#ifndef NDEBUG
     101    bool areDisabledAndScriptStatesConsistent() const;
     102#endif
     103
    100104    LinkLoader m_linkLoader;
    101105    CachedResourceHandle<CachedCSSStyleSheet> m_cachedSheet;
    … …  
    107111    LinkRelAttribute m_relAttribute;
    108112    bool m_loading;
    109     bool m_isEnabledViaScript;
     113    enum EnabledViaScriptState { Unset, EnabledViaScript, DisabledViaScript };
     114    EnabledViaScriptState m_scriptState;
    110115    bool m_createdByParser;
    111116    bool m_isInShadowTree;
    112    
     117
    113118    PendingSheetType m_pendingSheetType;
    114119};
Note: See TracChangeset for help on using the changeset viewer.