Skip to content

Commit

Permalink
2011-06-09 Julien Chaffraix <[email protected]>
Browse files Browse the repository at this point in the history 
        Reviewed by Antti Koivisto.

        REGRESSION(84329): Stylesheets on some pages do not load
        https://bugs.webkit.org/show_bug.cgi?id=61400

        Adding test to cover the regression. The test actually uncovered
        a bug in the way we handle alternate stylesheet and thus is
        failing some parts.

        * fast/css/link-disabled-attr-expected.txt: Added.
        * fast/css/link-disabled-attr.html: Added.
2011-06-09  Julien Chaffraix  <[email protected]>

        Reviewed by Antti Koivisto.

        REGRESSION(84329): Stylesheets on some pages do not load
        https://bugs.webkit.org/show_bug.cgi?id=61400

        Test: fast/css/link-disabled-attr.html

        Fixed r84329: the change did not take into account the fact
        that HTMLLinkElement did already contain the disabled information
        and the 2 information were not linked as they should have!

        The new logic pushes the information to the stylesheet as this
        is what the spec mandates and what FF is doing. Also it keeps
        one bit of information (that JS enabled the stylesheet) as it
        is needed for the recalcStyleSelector logic.

        * dom/Document.cpp:
        (WebCore::Document::recalcStyleSelector): s/isDisabled/disabled.

        * html/HTMLLinkElement.cpp:
        (WebCore::HTMLLinkElement::HTMLLinkElement): Removed m_disabledState,
        replaced by m_isEnabledViaScript.
        (WebCore::HTMLLinkElement::setDisabled): Updated the logic after
        m_disabledState removal. It also matches the spec by forwarding
        the disabled state to our stylesheet if we have one.
        (WebCore::HTMLLinkElement::parseMappedAttribute): Removed harmful
        handling of the disabledAttr.
        (WebCore::HTMLLinkElement::process): Updated after m_disabledState removal.
        * html/HTMLLinkElement.h:
        (WebCore::HTMLLinkElement::isEnabledViaScript): Ditto.
        (WebCore::HTMLLinkElement::isAlternate): Ditto.

Canonical link: https://commits.webkit.org/77896@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@88479 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
@webkit-commit-queue
webkit-commit-queue committed Jun 9, 2011
1 parent 38bbf12 commit b87afca
Show file tree
Hide file tree
Showing 7 changed files with 220 additions and 57 deletions.
14 changes: 14 additions & 0 deletions LayoutTests/ChangeLog
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
2011-06-09 Julien Chaffraix <[email protected]>

Reviewed by Antti Koivisto.

REGRESSION(84329): Stylesheets on some pages do not load
https://bugs.webkit.org/show_bug.cgi?id=61400

Adding test to cover the regression. The test actually uncovered
a bug in the way we handle alternate stylesheet and thus is
failing some parts.

* fast/css/link-disabled-attr-expected.txt: Added.
* fast/css/link-disabled-attr.html: Added.

2011-06-09 Julien Chaffraix <[email protected]>

Reviewed by Darin Adler.
Expand Down
27 changes: 27 additions & 0 deletions LayoutTests/fast/css/link-disabled-attr-expected.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
Series of tests to validate behavior of getting/setting link.disabled and link.sheet.disabled.
Test for bug 61400: REGRESSION(84329): Stylesheets on some pages do not load

notsheet
PASS link.sheet is null
PASS link.disabled is false
sheet
PASS link.sheet is non-null.
PASS link.disabled is true
PASS link.sheet.disabled is true
PASS getComputedStyle(console).whiteSpace is 'normal'
PASS link.disabled is false
PASS link.sheet.disabled is false
PASS getComputedStyle(console).whiteSpace is 'pre-wrap'
altsheet
FAIL link.disabled should be true. Was false.
PASS link.sheet is non-null.
FAIL getComputedStyle(console).backgroundColor should be rgb(0, 128, 0). Was rgba(0, 0, 0, 0).
FAIL link.disabled should be true. Was false.
PASS getComputedStyle(console).backgroundColor is originalBG
PASS link.disabled is false
FAIL getComputedStyle(console).backgroundColor should be rgb(0, 128, 0). Was rgba(0, 0, 0, 0).
PASS getComputedStyle(console).backgroundColor is originalBG
PASS successfullyParsed is true

TEST COMPLETE

103 changes: 103 additions & 0 deletions LayoutTests/fast/css/link-disabled-attr.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
<html>
<head>
<link id="sheet" rel="stylesheet" href="../js/resources/js-test-style.css">
<link id="notsheet" rel="author" href="mailto:[email protected]">
<link id="alt" rel="alternate stylesheet" title="altset" href="resources/green.css">
<script src="../js/resources/js-test-pre.js"></script>
</head>
<body>
<p id="description">
Series of tests to validate behavior of getting/setting link.disabled and link.sheet.disabled.<br>
Test for bug <a href="https://bugs.webkit.org/show_bug.cgi?id=61400">61400</a>: REGRESSION(84329): Stylesheets on some pages do not load
</p>
<div id="console"></div>
<script>

window.jsTestIsAsync = true;

function onSheetLoaded(f, elem, maxtime) {
if (elem.sheet || maxtime <= 0)
f(elem);
else
setTimeout(function () { onSheetLoaded(f, elem, maxtime - 25);}, 25);
}



// With a non-stylesheet <link>, 'disabled' is always false.

var console = document.getElementById("console");
var originalBG = getComputedStyle(console).backgroundColor;
var link;

debug("notsheet");

link = document.getElementById("notsheet");
shouldBeNull("link.sheet");
link.disabled = true;
shouldBeFalse("link.disabled");


// With a stylesheet <link>, 'disabled' and 'link.style.disabled' should both
// work, and be consistent with each other.

debug("sheet");

link = document.getElementById("sheet");
shouldBeNonNull("link.sheet");

link.sheet.disabled = true;
shouldBeTrue("link.disabled");
shouldBeTrue("link.sheet.disabled");
shouldBe("getComputedStyle(console).whiteSpace", "'normal'");

link.disabled = false;
shouldBeFalse("link.disabled");
shouldBeFalse("link.sheet.disabled");
shouldBe("getComputedStyle(console).whiteSpace", "'pre-wrap'");

link.sheet.disabled = false;


// An alternate stylesheet defaults to disabled when its title does not
// match the preferred set.

debug("altsheet");
link = document.getElementById("alt");
shouldBeTrue("link.disabled");

// Toggling link.disabled activates the stylesheet.

function altSheetLoaded(e) {
link = e;
shouldBeNonNull("link.sheet");
shouldBe("getComputedStyle(console).backgroundColor", "'rgb(0, 128, 0)'");

// Enabling a stylsheet set modifies disabled status of style sheets.

document.selectedStyleSheetSet = "nosuchset";
shouldBeTrue("link.disabled");
shouldBe("getComputedStyle(console).backgroundColor", "originalBG");

document.selectedStyleSheetSet = "altset";
shouldBeFalse("link.disabled");
shouldBe("getComputedStyle(console).backgroundColor", "'rgb(0, 128, 0)'");

// Disabling a stylesheet *after* its stylesheet set has been selected
// de-activates it.

link.disabled = true;
shouldBe("getComputedStyle(console).backgroundColor", "originalBG");

finishJSTest();
}

link.disabled = false;
onSheetLoaded(altSheetLoaded, link, 500);

successfullyParsed = true;

</script>
<script src="../js/resources/js-test-post.js"></script>
</body></html>
34 changes: 34 additions & 0 deletions Source/WebCore/ChangeLog
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,37 @@
2011-06-09 Julien Chaffraix <[email protected]>

Reviewed by Antti Koivisto.

REGRESSION(84329): Stylesheets on some pages do not load
https://bugs.webkit.org/show_bug.cgi?id=61400

Test: fast/css/link-disabled-attr.html

Fixed r84329: the change did not take into account the fact
that HTMLLinkElement did already contain the disabled information
and the 2 information were not linked as they should have!

The new logic pushes the information to the stylesheet as this
is what the spec mandates and what FF is doing. Also it keeps
one bit of information (that JS enabled the stylesheet) as it
is needed for the recalcStyleSelector logic.

* dom/Document.cpp:
(WebCore::Document::recalcStyleSelector): s/isDisabled/disabled.

* html/HTMLLinkElement.cpp:
(WebCore::HTMLLinkElement::HTMLLinkElement): Removed m_disabledState,
replaced by m_isEnabledViaScript.
(WebCore::HTMLLinkElement::setDisabled): Updated the logic after
m_disabledState removal. It also matches the spec by forwarding
the disabled state to our stylesheet if we have one.
(WebCore::HTMLLinkElement::parseMappedAttribute): Removed harmful
handling of the disabledAttr.
(WebCore::HTMLLinkElement::process): Updated after m_disabledState removal.
* html/HTMLLinkElement.h:
(WebCore::HTMLLinkElement::isEnabledViaScript): Ditto.
(WebCore::HTMLLinkElement::isAlternate): Ditto.

2011-06-09 Dan Bernstein <[email protected]>

Reviewed by Darin Adler.
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/dom/Document.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2967,7 +2967,7 @@ void Document::recalcStyleSelector()
if (e->hasLocalName(linkTag)) {
// <LINK> element
HTMLLinkElement* linkElement = static_cast<HTMLLinkElement*>(n);
if (linkElement->isDisabled())
if (linkElement->disabled())
continue;
enabledViaScript = linkElement->isEnabledViaScript();
if (linkElement->isLoading()) {
Expand Down
80 changes: 37 additions & 43 deletions Source/WebCore/html/HTMLLinkElement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ inline HTMLLinkElement::HTMLLinkElement(const QualifiedName& tagName, Document*
#if ENABLE(LINK_PREFETCH)
, m_onloadTimer(this, &HTMLLinkElement::onloadTimerFired)
#endif
, m_disabledState(Unset)
, m_loading(false)
, m_isEnabledViaScript(false)
, m_createdByParser(createdByParser)
, m_isInShadowTree(false)
, m_pendingSheetType(None)
Expand Down Expand Up @@ -85,40 +85,43 @@ HTMLLinkElement::~HTMLLinkElement()
#endif
}

void HTMLLinkElement::setDisabledState(bool _disabled)
void HTMLLinkElement::setDisabled(bool disabled)
{
DisabledState oldDisabledState = m_disabledState;
m_disabledState = _disabled ? Disabled : EnabledViaScript;
if (oldDisabledState != m_disabledState) {
// If we change the disabled state while the sheet is still loading, then we have to
// perform three checks:
if (isLoading()) {
// Check #1: The sheet becomes disabled while loading.
if (m_disabledState == Disabled)
removePendingSheet();

// Check #2: An alternate sheet becomes enabled while it is still loading.
if (m_relAttribute.m_isAlternate && m_disabledState == EnabledViaScript)
addPendingSheet(Blocking);

// Check #3: A main sheet becomes enabled while it was still loading and
// after it was disabled via script. It takes really terrible code to make this
// happen (a double toggle for no reason essentially). This happens on
// virtualplastic.net, which manages to do about 12 enable/disables on only 3
// sheets. :)
if (!m_relAttribute.m_isAlternate && m_disabledState == EnabledViaScript && oldDisabledState == Disabled)
addPendingSheet(Blocking);

// If the sheet is already loading just bail.
return;
}
if (!m_sheet)
return;

bool wasDisabled = m_sheet->disabled();
if (wasDisabled == disabled)
return;

m_sheet->setDisabled(disabled);
m_isEnabledViaScript = !disabled;

// If we change the disabled state while the sheet is still loading, then we have to
// perform three checks:
if (isLoading()) {
// Check #1: The sheet becomes disabled while loading.
if (disabled)
removePendingSheet();

// Check #2: An alternate sheet becomes enabled while it is still loading.
if (m_relAttribute.m_isAlternate && !disabled)
addPendingSheet(Blocking);

// Load the sheet, since it's never been loaded before.
if (!m_sheet && m_disabledState == EnabledViaScript)
process();
else
document()->styleSelectorChanged(DeferRecalcStyle); // Update the style selector.
// Check #3: A main sheet becomes enabled while it was still loading and
// after it was disabled via script. It takes really terrible code to make this
// happen (a double toggle for no reason essentially). This happens on
// virtualplastic.net, which manages to do about 12 enable/disables on only 3
// sheets. :)
if (!m_relAttribute.m_isAlternate && !disabled && wasDisabled)
addPendingSheet(Blocking);

// If the sheet is already loading just bail.
return;
}

if (!disabled)
process();
}

StyleSheet* HTMLLinkElement::sheet() const
Expand All @@ -140,9 +143,7 @@ void HTMLLinkElement::parseMappedAttribute(Attribute* attr)
} else if (attr->name() == mediaAttr) {
m_media = attr->value().string().lower();
process();
} else if (attr->name() == disabledAttr)
setDisabledState(!attr->isNull());
else if (attr->name() == onbeforeloadAttr)
} else if (attr->name() == onbeforeloadAttr)
setAttributeEventListener(eventNames().beforeloadEvent, createAttributeEventListener(this, attr));
#if ENABLE(LINK_PREFETCH)
else if (attr->name() == onloadAttr)
Expand Down Expand Up @@ -279,7 +280,7 @@ void HTMLLinkElement::process()

bool acceptIfTypeContainsTextCSS = document()->page() && document()->page()->settings() && document()->page()->settings()->treatsAnyTextCSSLinkAsStylesheet();

if (m_disabledState != Disabled && (m_relAttribute.m_isStyleSheet || (acceptIfTypeContainsTextCSS && type.contains("text/css")))
if (!disabled() && (m_relAttribute.m_isStyleSheet || (acceptIfTypeContainsTextCSS && type.contains("text/css")))
&& document()->frame() && m_url.isValid()) {

String charset = getAttribute(charsetAttr);
Expand Down Expand Up @@ -554,11 +555,4 @@ bool HTMLLinkElement::disabled() const
return m_sheet && m_sheet->disabled();
}

void HTMLLinkElement::setDisabled(bool disabled)
{
if (!m_sheet)
return;
m_sheet->setDisabled(disabled);
}

}
17 changes: 4 additions & 13 deletions Source/WebCore/html/HTMLLinkElement.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,9 @@ class HTMLLinkElement : public HTMLElement, public CachedResourceClient {

StyleSheet* sheet() const;

// FIXME: This should be remaned isStyleSheetLoading as this is only used for stylesheets.
bool isLoading() const;

bool isDisabled() const { return m_disabledState == Disabled; }
bool isEnabledViaScript() const { return m_disabledState == EnabledViaScript; }
bool isEnabledViaScript() const { return m_isEnabledViaScript; }
bool disabled() const;
void setDisabled(bool);

Expand All @@ -103,10 +102,8 @@ class HTMLLinkElement : public HTMLElement, public CachedResourceClient {
virtual bool sheetLoaded();
virtual void startLoadingDynamicSheet();

bool isAlternate() const { return m_disabledState == Unset && m_relAttribute.m_isAlternate; }
bool isAlternate() const { return m_relAttribute.m_isAlternate; }

void setDisabledState(bool _disabled);

virtual bool isURLAttribute(Attribute*) const;

public:
Expand All @@ -124,12 +121,6 @@ class HTMLLinkElement : public HTMLElement, public CachedResourceClient {
private:
HTMLLinkElement(const QualifiedName&, Document*, bool createdByParser);

enum DisabledState {
Unset,
EnabledViaScript,
Disabled
};

CachedResourceHandle<CachedCSSStyleSheet> m_cachedSheet;
RefPtr<CSSStyleSheet> m_sheet;
#if ENABLE(LINK_PREFETCH)
Expand All @@ -139,9 +130,9 @@ class HTMLLinkElement : public HTMLElement, public CachedResourceClient {
KURL m_url;
String m_type;
String m_media;
DisabledState m_disabledState;
RelAttribute m_relAttribute;
bool m_loading;
bool m_isEnabledViaScript;
bool m_createdByParser;
bool m_isInShadowTree;

Expand Down

0 comments on commit b87afca

Please sign in to comment.