Bug 148392 – Page does not update when <link> media attribute changes to no longer apply to page

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Joseph Pecoraro]

* SUMMARY
Page does not update when <link> media attribute changes to no longer apply to page.

* TEST
<link id="link" rel="stylesheet" href="data:text/css,body{color:red}" media="print">
<p>Text To Change Color</p>
<button id="button">Then Click me</button>
<script>
document.getElementById("button").addEventListener("click", function() {
    var link = document.getElementById("link");
    var currentMedia = link.media;
    var newMedia = currentMedia === "screen" ? "print" : "screen";
    console.log(currentMedia + " => " + newMedia);
    link.setAttribute("media", newMedia);
});
</script>

* STEPS TO REPRODUCE
1. Load test
2. Click the button (note the text changes from black => red)
3. Click the button again
  => text does not change from red => black, but it should

* NOTES
- Chrome and Firefox both behave as expected with this test case

[Joseph Pecoraro]

Triggering a style recalc (document().styleResolverChanged(DeferRecalcStyle)) when the media attribute changes does fix this, but there is probably something better we should be doing, such as only recalcing if the media query status (active/inactive) changes. Other tests should probably also be written (if the type changes without a rel attribute), etc.

[Joseph Pecoraro]

Created attachment 259785 [details]
[PATCH] Proposed Fix

[Chris Dumez]

Comment on attachment 259785 [details]
[PATCH] Proposed Fix

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

Just commenting on the layout test as I am not confident reviewing the fix. Somehow, it feels like process() should take care of this.

> LayoutTests/fast/css/link-media-attr.html:19
> +    // link.media is print

Useless if we use shouldBe below.

> LayoutTests/fast/css/link-media-attr.html:20
> +    debug("media=" + link.media);

shouldBeEqualToString("link.media", "print");

> LayoutTests/fast/css/link-media-attr.html:22
> +    shouldBe("getComputedStyle(target).color", "'rgb(0, 0, 0)'");

shouldBeEqualToString()

> LayoutTests/fast/css/link-media-attr.html:24
> +    link.media = "screen";

evalAndLog()

> LayoutTests/fast/css/link-media-attr.html:25
> +    debug("media=" + link.media);

shouldBeEqualToString("link.media", "screen");

> LayoutTests/fast/css/link-media-attr.html:27
> +    shouldBe("getComputedStyle(target).color", "'rgb(255, 0, 0)'");

shouldBeEqualToString()

> LayoutTests/fast/css/link-media-attr.html:29
> +    link.media = "screen,screen";

evalAndLog()

> LayoutTests/fast/css/link-media-attr.html:34
> +    link.media = "print";

evalAndLog()

> LayoutTests/fast/css/link-media-attr.html:35
> +    debug("media=" + link.media);

shouldBeEqualToString("link.media", "print");

> LayoutTests/fast/css/link-media-attr.html:37
> +    shouldBe("getComputedStyle(target).color", "'rgb(0, 0, 0)'");

shouldBeEqualToString()

[Joseph Pecoraro]

Created attachment 259937 [details]
[PATCH] Proposed Fix

[Antti Koivisto]

Comment on attachment 259937 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/html/HTMLLinkElement.cpp:172
>      if (name == mediaAttr) {
>          m_media = value.string().lower();
>          process();
> +        if (!isDisabled())
> +            document().styleResolverChanged(DeferRecalcStyle);
>          return;
>      }

Maybe only do this if the lowered value is actually different?

[Antti Koivisto]

Comment on attachment 259937 [details]
[PATCH] Proposed Fix

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

>> Source/WebCore/html/HTMLLinkElement.cpp:172
>>      }
> 
> Maybe only do this if the lowered value is actually different?

Also this shouldn't be needed if m_sheet is null.

[Joseph Pecoraro]

http://trac.webkit.org/changeset/189060