Closed Bug 461266 Opened 11 years ago Closed 11 years ago

error in final media query in list causes media query list to absorb everything until the next comma

Categories

(Core :: CSS Parsing and Computation, defect, major)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: smichaud, Assigned: dbaron)

References

()

Details

(Keywords: regression, testcase)

Attachments

(2 files, 1 obsolete file)

A Flash plugin loaded by this bug's URL (http://video.yahoo.com/)
displays in the wrong location (too high on the page), and overlays
other elements that it shouldn't overlay (including a couple of
tickers).

This bug is spun off from bug 459530 comment #45 and following, and
was originally reported by Tony Chung.  He also found the following
regression range for the bug, which is the same on OS X, Windows and
Linux:

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-10-11+02%3A00%3A00&enddate=2008-10-12+03%3A00%3A00

Nothing stands out (to me) in that list as the cause/trigger for this
bug.  But I wonder, dbaron, if any of the changes you landed on Sat
Oct 11 17:50:38 2008 -0700 might be responsible -- particularly
changeset 23eebebb8b48 (bug 322475, "Construct all our image loaders
when we create frames ...").

If my hunch is correct, this is a layout bug (and not a plugins bug).
Flags: wanted1.9.1?
Flags: blocking1.9.1?
i would doubt its a plugins bug, since it worked on 10/11 build but not 10/12.   this repros with both flash 9 and 10.
Component: Plug-ins → Layout
QA Contact: plugins → layout
Dbaron, can you take a look at this bug?   This should be a blocking 1.9.1 issue on video.yahoo.com, and needs to be fixed.   A regression range is on comment 0. Thanks, Tony
This bug has nothing to do with Flash. Afaics this is related to float. I'm preparing a minimized testcase.
Keywords: regression
I was able to reduce the data to a minimized testcase. It shows that a broken CSS declaration stops the parser from processing subsequent selectors. On the given website the line "@media all and(min-width:0)" gives a really huge warning which lists all subsequent selectors:

Warning: Expected ',' in media list but found 'and'.  Expected identifier in media list but found '#email_from'.  Expected identifier in media list but found '#email_msg'.  Expected identifier in media list but found '#player_embed'.  Expected identifier in media list but found '#addToPl'.  
[...]
Expected identifier in media list but found '#hottopics'.  Expected identifier in media list but found '#videoblog'.  Expected identifier in media list but found '#categories'.  Expected identifier in media list but found '#videoblog'.  Expected identifier for pseudo-class or pseudo-element but found '93%'.  Ruleset ignored due to bad selector.
Source File: file:///Users/henrik/Desktop/Yahoo%21%20Video%20-%20It%27s%20On_files/yv_global.css
Line: 406

Removing "and(min-width:0)" from the selector solves the problem and everything is displayed correctly.
Keywords: testcase
Summary: Flash plugin draws in wrong location → Broken CSS declaration stops parsing of subsequent selectors
David, your patch on bug 454226 caused this regression.
Assignee: nobody → dbaron
Blocks: 454226
Summary: Broken CSS declaration stops parsing of subsequent selectors → Broken "@media all" stops parsing of subsequent selectors
Assignee: dbaron → nobody
Component: Layout → Style System (CSS)
QA Contact: layout → style-system
Please don't assign bugs to people without their knowledge; that makes it look like they've agreed to work on them, but if they miss the bugmail they might not even know about them.
Assignee: nobody → dbaron
cc'ing adam, as he is writing tests for media query work
Opening this testcase should display a blue box at the right side of the document. Due to the changes in bug 454226 we stumble over the missing space in the query expression: "and(min-width:0)".
Attachment #345815 - Attachment is obsolete: true
Sorry for the bunch of summary updates. Now we should hopefully be fine.
Summary: Broken "@media all" stops parsing of subsequent selectors → Missing space in media query expression stops parsing of subsequent selectors
Hmmm, yeah, the "SkipUntil(',')" in CSSParserImpl::GatherMedia isn't right, since we should really be skipping until ',' or aStopSymbol.
Summary: Missing space in media query expression stops parsing of subsequent selectors → Broken "@media all" stops parsing of subsequent selectors
Summary: Broken "@media all" stops parsing of subsequent selectors → error in final media query in list causes media query list to absorb everything until the next comma
Attached patch patchSplinter Review
Included attachment 345818 [details] as a reftest; ran media queries mochitests (and had to make a fix to them that I should have had to make for bug 454226.
Attachment #345823 - Flags: superreview?(bzbarsky)
Attachment #345823 - Flags: review?(bzbarsky)
Flags: in-testsuite?
Attachment #345823 - Flags: superreview?(bzbarsky)
Attachment #345823 - Flags: superreview+
Attachment #345823 - Flags: review?(bzbarsky)
Attachment #345823 - Flags: review+
Fixed: http://hg.mozilla.org/mozilla-central/rev/01406b3b31c0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Flags: in-testsuite? → in-testsuite+
This seems to have lost us a point in the acid 3 test (test 46 case r) - does the acid test need to be updated or is it a actual regression?
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081103 Minefield/3.1b2pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081103 Minefield/3.1b2pre.   Thanks dbaron.
Status: RESOLVED → VERIFIED
Depends on: 462971
Flags: wanted1.9.1?
Flags: blocking1.9.1?
You need to log in before you can comment on or make changes to this bug.