Bug 83663

Summary: REGRESSION (r104445): Style is not recomputed on serenaandlily.com
Product: WebKit Reporter: Mike Lawther <mikelawther>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, kling, koivisto, macpherson, menard, webkit.review.bot
Priority: P1 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
reproduction
none
5 test cases (each being a single .html file) that illustrate the exact nature of the bug
none
patch
none
skip the subtree after calling setNeedsStyleRecalc on element
none
now with more correctness
none
different factoring, selector matching moves to SelectorChecker kling: review+, webkit.review.bot: commit-queue-

Description Mike Lawther 2012-04-10 23:12:54 PDT
Created attachment 136628 [details]
reproduction

As reported at http://crbug.com/122663:

A dynamically updated style is not recomputed, resulting in a page element appearing twice. This attached repro is not minimal, but it is breaking a live site.

Note that opening the inspector causes the page to recalculate and the error disappears.

I bisected WebKit Nightly on MacOS and got 

Works: r104428  Fails: r104450
http://trac.webkit.org/log/trunk/?rev=104450&stop_rev=104429

I reckon r104445 is the most likely in this range.
Comment 1 Alexey Proskuryakov 2012-04-11 13:38:23 PDT
<rdar://problem/11230095>
Comment 2 Antti Koivisto 2012-04-11 18:34:01 PDT
A minimal repro would be very helpful.
Comment 3 Warren 2012-04-14 15:56:12 PDT
Created attachment 137220 [details]
5 test cases (each being a single .html file) that illustrate the exact nature of the bug

As reported at http://crbug.com/122663:

The issue boils down to css identifiers with a value that uses CamelCase!

I've attached 5 (relatively) minimal tests that illustrate this behavior.
They only differ by a few characters.. so the easiest way to see what has been tweaked is by looking at a diff, but I'll summarize.

test #1 (pass):
  the css id="div_lowercase"
test #2 (fail):
  the css id="div_CamelCase"
test #3 (fail):
  the css id="div_CamelCase"
  addendum:
  * test #2 wasn't concerned with bumping up the css width of #div_02
    the first css block sets its width = 729px
    which is why when #div_CamelCase is (incorrectly) still visible,
    the 2 floats can still fit side by side without vertically wrapping.
  * test #3 illustrates that we can add a rule
    in the same <style> block that contains the rule to hide #div_CamelCase,
    which is being ignored,
    to increase the width of #div_02.
    this is an important test because all selectors in the 1st style tag
    specify a more precise class hierarchy and (consequently) take precedence.
    both of the rules in the 2nd style block use "!important" to over rule.
    this test shows that this is not the issue,
    because it works for one but not the other (ie: the one with CamelCase).
test #4 (pass):
  the css id="div_CamelCase"
  addendum:
  * test #4 is fun because:
    instead of using "!important" in the 2nd style block to over rule the 1st,
    #div_02 uses the same class hierarchy to take precedence (since it occurs later).
    what's really weird here,
    is that even though the rule to hide #div_CamelCase has NOT been changed,
    and the rule that has been change is effectively equivalent to test #3,
    #div_CamelCase is NOW HIDDEN.
test #5 (pass):
  the css id="div_CamelCase"
  addendum:
  * test #5 is a slight modification on what was shown in #4
  * the width of #div_02 is ignored (as it was in #2)
  * the selector in the 2nd style block for #div_CamelCase is made more explicit,
    and suddenly it is obeyed!

so.. long story short:
  * somewhere in WebKit..
    case sensitivity has been introduced in css selector parsing.
  * when a css id is impacted by this bug,
    there are some weird ways to work around it.

hope this helps..
Comment 4 Antti Koivisto 2012-04-15 23:51:36 PDT
Created attachment 137284 [details]
patch
Comment 5 Antti Koivisto 2012-04-16 00:11:01 PDT
Created attachment 137290 [details]
skip the subtree after calling setNeedsStyleRecalc on element
Comment 6 Antti Koivisto 2012-04-16 00:25:31 PDT
Created attachment 137293 [details]
now with more correctness
Comment 7 Antti Koivisto 2012-04-16 01:11:13 PDT
Created attachment 137297 [details]
different factoring, selector matching moves to SelectorChecker
Comment 8 WebKit Review Bot 2012-04-16 01:31:06 PDT
Comment on attachment 137297 [details]
different factoring, selector matching moves to SelectorChecker

Attachment 137297 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12406960
Comment 9 Antti Koivisto 2012-04-16 09:38:34 PDT
http://trac.webkit.org/changeset/114265 (with an added #include for chromium)