Closed Bug 1132745 Opened 5 years ago Closed 4 years ago

Finalize the contents of the -webkit prefix emulation "fixlist"/"whitelist"

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla40
Tracking Status
firefox39 --- affected
firefox40 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files, 1 obsolete file)

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.)
Summary: Finalize the contents of the -wekbit prefix emulation "fixlist" → Finalize the contents of the -wekbit prefix emulation "fixlist"/"whitelist"
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
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
Flags: needinfo?(dholbert)
(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)
Flags: needinfo?(pcheng) → needinfo?(pcheng)
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)
Summary: Finalize the contents of the -wekbit prefix emulation "fixlist"/"whitelist" → Finalize the contents of the -webkit prefix emulation "fixlist"/"whitelist"
(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.
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)
(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)
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 on attachment 8601317 [details] [diff] [review]
part 1: remove music.baidu.com

Review of attachment 8601317 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.
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.
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/80b09ba3d52e
https://hg.mozilla.org/mozilla-central/rev/169878ee34f9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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)
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).
(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).
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)
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)
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!
(^ is the followup which should fix m.kuaidi100.com.)
I verified m.kuaidi100.com on latest nightly today (build 201510030207). The layout looks good now. Thank you all!
Status: RESOLVED → VERIFIED
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.
You need to log in before you can comment on or make changes to this bug.