Bug 91058 - Modifying an attribute for which there is a selector in html.css does not cause a style recalc
Summary: Modifying an attribute for which there is a selector in html.css does not cau...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 90931
  Show dependency treegraph
 
Reported: 2012-07-12 01:33 PDT by Matt Falkenhagen
Modified: 2017-07-18 08:30 PDT (History)
10 users (show)

See Also:


Attachments
selector testcase (2.19 KB, patch)
2012-07-13 00:23 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
better testcase that uses <pre> and <code> instead of <span> (3.00 KB, patch)
2012-07-19 02:12 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Falkenhagen 2012-07-12 01:33:32 PDT
Modifying an attribute for which there is a selector in html.css does not initially cause a style recalc. But once a recalc is forced (e.g., by zooming in/out or opening debug console), modifying the attribute thereafter will cause the style recalc to happen.

In practice, the bug does not occur in Safari, but it does occur when the layout tests are run. The bug occurs on Chromium in the browser itself and layout tests.

I believe the problem is the Document's StyleResolver does not yet have m_features.attrsInRules populated, so the code in Element::attributeChanged doesn't cause a style recalc. Once the recalc is forced (as mentioned above), the attributeChanged will trigger the recalc.

I discovered this in the patch for bug 90931 (https://bug-90931-attachments.webkit.org/attachment.cgi?id=151653). I tried to come up with a simpler test case, but it seems the relevant attributes in html.css already have special code to trigger a recalc. For example, iframe seamless has code in HTMLIFrameElement::parseAttribute:

  // If we're adding or removing the seamless attribute, we need to force the content document to recalculate its StyleResolver.
  if (contentDocument())
      contentDocument()->styleResolverChanged(DeferRecalcStyle);

I'll see if I can create a simpler test case apart from bug 90931 later.
Comment 1 Alexey Proskuryakov 2012-07-12 13:17:44 PDT
Sounds like this could also affect audio element rendering when controls attribute is added or removed.
Comment 2 Eric Seidel (no email) 2012-07-12 13:41:35 PDT
I think the seamless example is a red-herring. :)  switching into and out of seamless mode is a big change, causing you to need to recalculate your root style (which you normally don't do).

I'm interested in this bug, but I'm not sure I understand what your describing yet.  Do you have a test case?
Comment 3 Matt Falkenhagen 2012-07-13 00:23:59 PDT
Created attachment 152167 [details]
selector testcase
Comment 4 Matt Falkenhagen 2012-07-13 00:26:32 PDT
I've attached a patch that illustrates what I'm seeing, i.e., the layout test doesn't have the desired results when run with run_webkit_tests or new-run-webkit-tests.

I added to html.css to illustrate this as I couldn't get the bug to occur with existing attribute selectors.

When this particular layout test is opened directly in the browser, as opposed to the layout test script, it seems like the bug doesn't occur. But with similar testcases, I get inconsistent results in Chrome: sometimes I see the bug and sometimes not.

Oddly, the bug occurs even without dynamically changing the attribute, as seen in the second <span> of the layout test.
Comment 5 Matt Falkenhagen 2012-07-19 02:12:27 PDT
Created attachment 153211 [details]
better testcase that uses <pre> and <code> instead of <span>

I discovered the previous testcase was bad since <span> uses the simple default style in StyleResolver.cpp (elementCanUseSimpleDefaultStyle() returns true).

I've modified the testcase to use <pre> and <code> instead. In this test case, an attribute selector pre[open] is defined in html.css and a code[open] one is defined in the layout test itself. When I run the test, <pre>'s does not take effect, but <code>'s does.

I found that changing elementCanUseSimpleDefaultStyle() to always return false seems to fix the bug, but I'm not sure that is the real cause. If elementCanUseSimpleDefaultStyle() is not changed and instead the pre[open] CSS is simply added to the simpleUserAgentStyleSheet string, the bug still occurs.

It seems it is some timing issue of when the stylesheet is read. It seems if I run a fresh instance of Chromium and immediately open the layout test, it fails, but if I then reload or open the test by clicking a link to it, it passes. Also, running other layout tests before this one causes it to pass; it only fails if it's the only test run.
Comment 6 Pravin D 2012-08-14 00:29:50 PDT
Matt , is it possible to attach a test case as a html file n without the html.css changes ?
Comment 7 Matt Falkenhagen 2012-08-14 19:16:14 PDT
(In reply to comment #6)
> Matt , is it possible to attach a test case as a html file n without the html.css changes ?

I wasn't able to get the bug to occur the existing selectors in html.css (see comment #4)