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

VERIFIED FIXED in Firefox 40

Status

()

Core
CSS Parsing and Computation
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla40
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox39 affected, firefox40 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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

2 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

2 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
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

2 years ago
Flags: needinfo?(dholbert)
(Assignee)

Comment 3

2 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

2 years ago
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)
(Assignee)

Updated

2 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

2 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

2 years ago
Created attachment 8601317 [details] [diff] [review]
part 1: remove music.baidu.com
(Assignee)

Comment 7

2 years ago
Created attachment 8601318 [details] [diff] [review]
part 2: add domains that serve the relevant prefixed styles for requested sites
(Assignee)

Comment 8

2 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

2 years ago
Created attachment 8601332 [details] [diff] [review]
part 2 v2: add domains that serve the relevant prefixed styles for requested sites

(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

2 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 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.

Comment 13

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/80b09ba3d52e
https://hg.mozilla.org/integration/mozilla-inbound/rev/169878ee34f9
(Assignee)

Updated

2 years ago
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/80b09ba3d52e
https://hg.mozilla.org/mozilla-central/rev/169878ee34f9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox40: --- → fixed
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).
(Assignee)

Comment 17

2 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).
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

2 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

2 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!

Comment 21

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/60ab1ea8f158
(Assignee)

Comment 22

2 years ago
(^ is the followup which should fix m.kuaidi100.com.)
https://hg.mozilla.org/mozilla-central/rev/60ab1ea8f158
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.
status-firefox39: --- → affected
You need to log in before you can comment on or make changes to this bug.