Closed
Bug 1132745
Opened 9 years ago
Closed 9 years ago
Finalize the contents of the -webkit prefix emulation "fixlist"/"whitelist"
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla40
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(2 files, 1 obsolete file)
1.17 KB,
patch
|
miketaylr
:
review+
|
Details | Diff | Splinter Review |
1.63 KB,
patch
|
miketaylr
:
review+
|
Details | Diff | Splinter Review |
This bug is on finalizing the contents of the (short) "fixlist" of sites where we'll be emulating some -webkit prefixed properties/values. (I'm adding support for a fixlist in bug 1132743, probably with a few entries to start, but we'll add the rest of the entries here.)
Assignee | ||
Updated•9 years ago
|
Summary: Finalize the contents of the -wekbit prefix emulation "fixlist" → Finalize the contents of the -wekbit prefix emulation "fixlist"/"whitelist"
Assignee | ||
Comment 1•9 years ago
|
||
Note: currently, the whitelist lives here (for whitelisted full domains): http://mxr.mozilla.org/mozilla-central/source/caps/nsPrincipal.cpp?rev=14b7ae148c8f#637 and here (for whitelisted subdomains within a base domain): http://mxr.mozilla.org/mozilla-central/source/caps/nsPrincipal.cpp?rev=14b7ae148c8f#670
Comment 2•9 years ago
|
||
Hi Daniel, I tested top 100 China websites (from Chinaz.com and alexa.com) and here is a list of website that have critical css layout problems. Could you please help add them to the white list so we can check whether unprefixing can fix them? BTW, music.baidu.com is ok now, so I think we can remove it from the white list. # website to add: sina.cn m.iqiyi.com m.kuaidi100.com m.jiaodong.net m.pc6.com m.haosou.com m.mi.com wappass.baidu.com video.baidu.com m.v.qq.com # website to remove: music.baidu.com Here is the detailed test results: https://docs.google.com/spreadsheets/d/1eP1eOJC7YHvj5EW_MsPfughtKrTFFPIxVLAXeZYJD1M/edit?usp=sharing
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(dholbert)
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Peipei Cheng (needinfo if you need my action) from comment #2) > Could you please help add them to the white list so we can check whether > unprefixing can fix them? For "check whether unprefixing can fix them" purposes, I'd suggest using Mike's try build (which has the whitelist disabled), linked in bug 1158383 comment 0. Direct link to build directory: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mitaylor@mozilla.com-9cd5527b9aa0/try-android-api-11/ (and the file to get is "fennec-40.0a1.en-US.android-arm.apk" I think)
Flags: needinfo?(dholbert) → needinfo?(pcheng)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(pcheng) → needinfo?(pcheng)
Comment 4•9 years ago
|
||
Thank you Daniel! I tested using Mike's try build, and following website pass test. I think we could add them to the whitelist. Any problem, please let me know. # website to add: sina.cn m.iqiyi.com m.kuaidi100.com m.pc6.com m.haosou.com m.mi.com wappass.baidu.com video.baidu.com m.v.qq.com # website to remove: music.baidu.com
Flags: needinfo?(pcheng) → needinfo?(dholbert)
Assignee | ||
Updated•9 years ago
|
Summary: Finalize the contents of the -wekbit prefix emulation "fixlist"/"whitelist" → Finalize the contents of the -webkit prefix emulation "fixlist"/"whitelist"
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Peipei Cheng (needinfo if you need my action) from comment #4) > Any problem, please > let me know. Notes on remaining brokenness in 2 sites below: > m.kuaidi100.com Observation: For me (spoofing a Firefox for android user agent & responsive design mode), this site loads with a nearly full-screen broken-image icon, regardless of unprefixing service. On Chrome, the broken-image icon is at the bottom of the screen and doesn't cover up much. This seems to be due to a difference between Chrome & Firefox in broken-image icon sizing. Reduced testcase: data:text/html,<div%20style="position:fixed;width:100%;bottom:0"><img%20src="https://bugzilla.mozilla.org/bogus-image-url.jpg"%20width="100%"> Basically, we seem to treat the broken-image icon as having a 1:1 intrinsic aspect ratio, and so "width:100%" gives us an equally large height. Chrome apparently does not treat the icon as having an intrinsic aspect ratio, as shown by that reduced testcase. > video.baidu.com Two notes on this one: (1) I think you mean http://m.video.baidu.com/ , right? (that's what I get redirected to, if I visit video.baidu.com with a Firefox for Android user agent) (2) Observation: the header on this page gets significantly worse w/ the unprefixing service, due to: - their use of a "-webkit-box" container to hold a "slider", with a lot of elements, only one of which is intneded to be displayed at a time (and the rest of which are positioned offscreen via "transform") - the fact that modern flexbox shrinks its items by default (via default "flex-shrink:1"), to try to make them fit. (I don't know if there's any straightforward workaround for this sort of thing.) The rest of the layout seems to improve, though, so this is probably still a net improvement.
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8601317 [details] [diff] [review] part 1: remove music.baidu.com dbaron & bz reviewed the fixlist implementation in bug 1132743, but I don't want to bother them with this review, since it's pretty trivial and their review-time is generally in-demand. Mike, would you be up for reviewing? Literally just removing a line and adding a few more; just a quick sanity-check should do.
Flags: needinfo?(dholbert)
Attachment #8601317 -
Flags: review?(miket)
Assignee | ||
Comment 9•9 years ago
|
||
(Reuploading part 2 w/ a commit-message tweak -- just adding a note that these sites were requested by Mozilla China team.)
Attachment #8601318 -
Attachment is obsolete: true
Attachment #8601332 -
Flags: review?(miket)
Assignee | ||
Comment 10•9 years ago
|
||
FWIW, I tested part 2 locally (on Desktop, spoofing a mobile UA & using responsive-design-mode) and verified that comment 4's sites are all improved with this patch. (modulo the notes in comment 5)
Comment 11•9 years ago
|
||
Comment on attachment 8601317 [details] [diff] [review] part 1: remove music.baidu.com Review of attachment 8601317 [details] [diff] [review]: ----------------------------------------------------------------- lgtm.
Comment 12•9 years ago
|
||
Comment on attachment 8601332 [details] [diff] [review] part 2 v2: add domains that serve the relevant prefixed styles for requested sites Review of attachment 8601332 [details] [diff] [review]: ----------------------------------------------------------------- Also LGTM. I think we should go back and add bug numbers to track removing these domains from the whitelist (hopefully we have TE bugs for these domains anyways)--but I can do that at a later point.
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/80b09ba3d52e https://hg.mozilla.org/integration/mozilla-inbound/rev/169878ee34f9
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite-
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/80b09ba3d52e https://hg.mozilla.org/mozilla-central/rev/169878ee34f9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 15•9 years ago
|
||
Daniel, I tested these sites in latest nightly build. All sites are improved except the problems you mentioned in comment 5. For m.kuaidi100.com, it displays well using mike's build. So I think there might be something we can do. Could you please help take a look again?
Flags: needinfo?(dholbert)
Comment 16•9 years ago
|
||
I'm going to be looking at top Japan sites today for unprefixing, so I can take a look at m.kuaidi100.com as well (to save Daniel the context switching).
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Peipei Cheng (needinfo if you need my action) from comment #15) > For m.kuaidi100.com, it displays well using mike's build As I recall, it didn't for me (it had the large broken-image icon). I wasn't directly using Mike's when I hit this, but I was testing effectively the same thing -- a build with the whitelist completely disabled. Have you re-tested Mike's build, after testing the new Nightly, and verified that it doesn't hit the large-broken-icon issue from comment 5? If not, I'm guessing this behavior-difference may be due to a server-side change (that image going missing, in particular).
Comment 18•9 years ago
|
||
I see a lot of (old) webkit flexbox in http://cdn.kuaidi100.com/css/smart/query.css?version=201504201501', so it may be that they moved some things around. (Somewhat related is we should probably have some kind of automation to verify that whitelisted stylesheets continue to live where we think they live. I think Hallvord has some scripts to do this already)
Assignee | ||
Comment 19•9 years ago
|
||
Firstly, I don't see the huge broken-image-icon anymore, in any configuration (tested no whitelisting & current updated whitelist), so I think they fixed that server-side. Secondly, yeah, the site layout is still broken -- that's probably what Peipei was talking about in comment 12. (And as Mike says in comment 18, it looks like the styles are coming from cdn.kuaidi100.com, whereas I whitelisted www.kuaidi100.com.) > it may be that they moved some things around Or I might've just made a mistake when copypasting the domain. (It looks like their logo at least is served from www., so I probably just copied the wrong domain from their source, and then didn't notice when re-testing because their page was covered up with the broken-image icon.)
Flags: needinfo?(dholbert)
Assignee | ||
Comment 20•9 years ago
|
||
I think we need s/www/cdn/ in the whitelist, probably. I'll sanity-check locally that that fixes things, & push that as a followup for this bug, if it does. (I'm pushing another patch in a bit anyway, so I'll bundle it together.) Thanks for catching that, Peipei, and thanks for taking a look Mike!
Assignee | ||
Comment 22•9 years ago
|
||
(^ is the followup which should fix m.kuaidi100.com.)
Comment 24•9 years ago
|
||
I verified m.kuaidi100.com on latest nightly today (build 201510030207). The layout looks good now. Thank you all!
Status: RESOLVED → VERIFIED
Comment 25•9 years ago
|
||
Is it possible to pick up this fix in v39.0 since the css un-prefixing service is already in v39.0? I think there should be no risk.
status-firefox39:
--- → affected
You need to log in
before you can comment on or make changes to this bug.
Description
•