Closed
Bug 875305
Opened 11 years ago
Closed 11 years ago
Bug 833388 added potentially-slow rules to html.css
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
People
(Reporter: bzbarsky, Assigned: rillian)
References
Details
(Keywords: regression)
Attachments
(2 files, 4 obsolete files)
2.80 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.37 KB,
patch
|
bzbarsky
:
review+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
>+video > div {
>+video > div p {
These should be using classes on the right-hand-most thing if at all possible, imo. It's possible the Bloom filter saves the day, but I'd rather we didn't depend on that.
Comment 1•11 years ago
|
||
Asking in IRC to know if Jesse is able to take this on, but not tracking unless we can get an assignee to work on this.
Comment 2•11 years ago
|
||
If we can't get a forward fix, what is the risk of backing out bug 833388 and bringing it back at a later time with a re-working of these rules?
Comment 3•11 years ago
|
||
bz are you able to assess the question in comment 2?
Flags: needinfo?(bzbarsky)
Comment 4•11 years ago
|
||
So those rules were added specifically for WEBVTT text. Right now they are only temporary. We've opened a bug https://bugzilla.mozilla.org/show_bug.cgi?id=876505 to get the CSS styling for WEBVTT up to date with the spec. I don't know how important it would be to update the CSS here as it will be changing very soon in accordance with the spec. This temporary CSS won't be seeing much use either as WEBVTT is still behind a pref. Maybe it would be better to just ensure that the CSS that lands in the other bug uses fast rules.
Reporter | ||
Comment 5•11 years ago
|
||
> So those rules were added specifically for WEBVTT text. Right now they are only > temporary. My point is that these rules affect the performance of styling _all_ web content, in all builds that have that change. Including builds that have webvtt disabled. I don't think that's acceptable. > what is the risk of backing out bug 833388 I think the risk of backing out the html.css changes from that bug is zero (since they don't actually _match_ anything unless webvtt is enabled, and it's not enabled) and that this is in fact what we should do ASAP for at least 22 and 23.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 7•11 years ago
|
||
Switch to using unprefixed class selectors instead, as recommended by https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS Can we use a leading underscore for class names to reduce the chance of collision?
Assignee: rick.eyre → giles
Assignee | ||
Comment 8•11 years ago
|
||
or a leading dash, I guess. There's no point is setting moz-caption-box on the anonymous div, since bug 833382 changes the css properties to use something inside that div as the caption box itself. Rick, does this look reasonable to you?
Comment 9•11 years ago
|
||
Looks great to me. It works when applied to bug833382. Much simpler solution then I had in mind too!
Assignee | ||
Comment 10•11 years ago
|
||
Updated patch. There's no need to set the class on the top-level caption div. This is just a container for the caption box div, which must be positioned inside the video frame. The patch from bug 833382 will need to be updated to set the proper class names on the elements it creates.
Attachment #754870 -
Attachment is obsolete: true
Attachment #754911 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 11•11 years ago
|
||
Comment on attachment 754911 [details] [diff] [review] use classes instead No, this is not ok either, since it will match things on web pages. However, |video > .moz-caption-box| and |video > div .moz-caption-text| would probably be ok. Still detectable by web pages, but no worse than stuff in forms.css. > Can we use a leading underscore for class names to reduce the chance of > collision? You can use any Unicode character you want to in class names in CSS. In HTML you can use any Unicode character except ASCII spaces.
Attachment #754911 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 754870 [details] [diff] [review] use classes instead Rick prefers this patch. Sorry for the confusion.
Attachment #754870 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 13•11 years ago
|
||
Comment on attachment 754870 [details] [diff] [review] use classes instead bugzilla lag.
Attachment #754870 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•11 years ago
|
||
Updated patch. - Keep 'video >' parent so we can't affect page content - Drop the 'moz-' prefix from the caption names since they're restricted to video children. - Restore the class on the top-level anonymous div. per reyre's comments on irc.
Attachment #754911 -
Attachment is obsolete: true
Attachment #754935 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 15•11 years ago
|
||
Comment on attachment 754935 [details] [diff] [review] use classes on the rightmost selector >+ nsRefPtr<nsGenericHTMLElement> div = do_QueryObject(mCaptionDiv); >+ if (div) { That QI would result in a non-null pointer for any element on the RHS, since nsGenericHTMLElement does not have its own IID. So please don't to that: it's wrong. Instead, just static_cast mCaptionDiv to nsGenericHTMLElement*, since you know it is in fact one. r=me with that and assuming that you don't need the caption-text class on anything yet. Please don't forget to land this on the relevant branches!
Attachment #754935 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 16•11 years ago
|
||
Fix nits: - static_cast instead of QueryObject so we can call SetClassName. - caption-text isn't used by code in m-c. bug 833382 will use it. Carrying forward earlier r+
Attachment #754935 -
Attachment is obsolete: true
Assignee | ||
Comment 17•11 years ago
|
||
Green on try. https://tbpl.mozilla.org/?tree=Try&rev=0230403da169
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•11 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 18•11 years ago
|
||
Fix commit message git->hg noise.
Attachment #754997 -
Attachment is obsolete: true
Assignee | ||
Comment 19•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/db50ad5872a2
Keywords: checkin-needed
Comment 20•11 years ago
|
||
Can we backout bug 833388 for FF22 and take this forward fix for FF23? What's the risk of the forward fix here? Let's try to get a branch fix in this week.
Assignee | ||
Comment 21•11 years ago
|
||
Comment on attachment 755168 [details] [diff] [review] apply classes to rightmost selector [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 833388 User impact if declined: Users may see slower page loads. Testing completed (on m-c, etc.): Landed on m-i, green on try. Risk to taking this patch (and alternatives if risky): Low. The CSS rules are for code which isn't present in Aurora. String or IDL/UUID changes made by this patch: none. Patch applied to aurora and pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=d5302ee47a16
Attachment #755168 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 22•11 years ago
|
||
Alex, that makes sense to me. I'll prepare a patch just removing the css rules for Beta. That should be safer than backing out the code change as well.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 23•11 years ago
|
||
[Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 833388 User impact if declined: Users may experience slower page rendering. Testing completed (on m-c, etc.): Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=fa8c6cb6af08 Risk to taking this patch (and alternatives if risky): Risk it minimal; the patch just removes unused CSS rules from the default stylesheet. String or IDL/UUID changes made by this patch: none.
Attachment #755658 -
Flags: review?(bzbarsky)
Attachment #755658 -
Flags: approval-mozilla-beta?
Reporter | ||
Comment 24•11 years ago
|
||
Comment on attachment 755658 [details] [diff] [review] remove caption CSS from beta r=me
Attachment #755658 -
Flags: review?(bzbarsky) → review+
Comment 26•11 years ago
|
||
(In reply to Ralph Giles (:rillian) from comment #23) > Testing completed (on m-c, etc.): Pushed to try as > https://tbpl.mozilla.org/?tree=Try&rev=fa8c6cb6af08 Would you mind reviewing the failures and ensuring that none of them are related to this patch?
Updated•11 years ago
|
Attachment #755168 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 27•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ddc11868e236
Assignee | ||
Comment 28•11 years ago
|
||
(In reply to Alex Keybl [:akeybl] from comment #26) > > https://tbpl.mozilla.org/?tree=Try&rev=fa8c6cb6af08 > > Would you mind reviewing the failures and ensuring that none of them are > related to this patch? I've starred a number of them on the beta push. The Android red is I think just broken; changes to html.css can't make the configure script go missing and those aren't running on beta commits in tbpl. The rest aren't obviously related, but I didn't dig too deeply. I can look more closely, but probably not before Tuesday. I've respun some of those to measure the repeatability.
Comment 29•11 years ago
|
||
Please let us know how things turned out - we may end up landing blindly to mozilla-beta and wait for results if your investigation goes past Tuesday (our b4 go-to-build).
Comment 30•11 years ago
|
||
FWIW I don't really think any of these failures could be caused by the patch as the CSS that was removed was only being used by WEBVTT which is currently preffed off and the pieces that use it haven't landed in the tree yet.
Comment 31•11 years ago
|
||
Comment on attachment 755658 [details] [diff] [review] remove caption CSS from beta Let's land and backout if necessary, since we suspect this will build/test cleanly.
Attachment #755658 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 32•11 years ago
|
||
Thanks. Pushed to beta in https://hg.mozilla.org/releases/mozilla-beta/rev/18cbe54e3c88
Assignee | ||
Comment 33•11 years ago
|
||
This should be resolved on all branches now.
status-firefox22:
--- → fixed
status-firefox23:
--- → fixed
status-firefox24:
--- → fixed
Whiteboard: [leave open]
Assignee | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
status-firefox22:
fixed → ---
status-firefox23:
fixed → ---
status-firefox24:
fixed → ---
OS: All → Mac OS X
Hardware: All → x86
Resolution: --- → FIXED
Assignee | ||
Comment 34•11 years ago
|
||
Thank you stale cache interaction, and dbaron for noticing. I've reset the status flags, but don't have permission to change the tracking flags back to '+' for firefox 22,23,24. Could someone please reset those?
status-firefox22:
--- → fixed
status-firefox23:
--- → fixed
status-firefox24:
--- → fixed
tracking-firefox24:
? → ---
OS: Mac OS X → All
Hardware: x86 → All
You need to log in
before you can comment on or make changes to this bug.
Description
•