Closed
Bug 760098
Opened 12 years ago
Closed 12 years ago
Default (minTwips; 120) text-inflation on YouTube makes us look bad
Categories
(Tech Evangelism Graveyard :: English US, defect, P1)
Tracking
(blocking-kilimanjaro:+, firefox14 verified, firefox15+ verified, firefox16 verified, blocking-fennec1.0 +)
VERIFIED
FIXED
blocking-kilimanjaro | + |
People
(Reporter: aaronmt, Assigned: bugs)
References
()
Details
(Keywords: regression, Whiteboard: [readability][special case patch in comment 28])
Attachments
(3 files)
956.29 KB,
image/png
|
Details | |
6.72 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
1.29 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
See screenshot. This is a regression as 14.0 Beta is unaffected. Samsung Galaxy Nexus (Android 4.0.4) 20120531040223 http://hg.mozilla.org/integration/mozilla-inbound/rev/ff2ad97929e8
Reporter | ||
Updated•12 years ago
|
Blocks: youtube.com
Reporter | ||
Comment 1•12 years ago
|
||
This issue is present on Aurora (tested via: 05/31) actually with bug 747720#c17. http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1158503601be&tochange=c20d415ef1b5 Regression from bug 747720.
Blocks: 747720
Reporter | ||
Updated•12 years ago
|
Keywords: regressionwindow-wanted
Reporter | ||
Updated•12 years ago
|
Component: General → Layout
Product: Fennec Native → Core
QA Contact: general → layout
Version: unspecified → Trunk
Reporter | ||
Updated•12 years ago
|
Reporter | ||
Updated•12 years ago
|
Keywords: regressionwindow-wanted
Reporter | ||
Updated•12 years ago
|
Keywords: testcase-wanted
Updated•12 years ago
|
blocking-kilimanjaro: --- → ?
Updated•12 years ago
|
blocking-fennec1.0: ? → +
Assignee | ||
Comment 2•12 years ago
|
||
To David for a look. Regression from bug 747720.
Assignee: nobody → dbaron
Updated•12 years ago
|
Whiteboard: [readability]
Updated•12 years ago
|
probably need to back out last patch (overflow) from bug 747720
Updated•12 years ago
|
Priority: -- → P1
or, alternately, we could use the inflation threshold to have require contiguous content (but not looking through placeholders) to pass the threshold. This could make the threshold checks a lot slower, though. I think there was something else I thought of during the meeting that was better than either of those, though, but I've forgotten what it was (though maybe it was just the above).
I think this particular case would be fixed by ignoring text in containers that we know isn't going to be inflated, though.
(since on youtube the containers with overflow:hidden also have max-height)
Comment 7•12 years ago
|
||
There was a talk of a white or black list.
No, I thought of a better solution, but now I've forgotten.
Updated•12 years ago
|
blocking-kilimanjaro: ? → +
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/378b60ffc24f/line-threshold-check-inflated doesn't actually work, unless I'm doing something wrong. (Backing out the overflow part of bug 747720 does, though.)
Comment 10•12 years ago
|
||
It sounds, from speaking with Jet, that for this beta release, one thing we could do is blacklist the youtube site, and simply not use font inflation on that site. I'm not sure how to implement this type of blacklist capability in the layout engine, like we do for graphics. (I guess I'm just not entirely sure on the details of what happens - so, in my mind we could have some text file with blacklisted sites in a regex form, e.g. *.youtube.com, and then when the document is first constructed, set an internal static variable, say, nsLayoutUtils::sForceFontInflationDisabled to true if the URI matches any of the regexs that we read in from this file). I think this is, at the very best, a band-aid fix, and we'd want to remove it immediately from central to not get tempted to utilize it for a bunch of sites that we really should be fixing the behavior for.
Oh, I think the other idea was to require contiguous text to meet the line threshold. (Probably "contiguous" means "within the same block".)
(In reply to David Baron [:dbaron] from comment #11) > Oh, I think the other idea was to require contiguous text to meet the line > threshold. (Probably "contiguous" means "within the same block".) The problem with this is it might be a performance problem, since we'd end up scanning large amounts of text to see if we have a piece that meets the threshold. (We don't know, since we scan at most the amount that causes us to hit the threshold.)
Attachment #630756 -
Flags: review?(roc)
Attachment #630756 -
Flags: review?(roc) → review+
Comment 15•12 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/cca15a0b49f3 for https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=30e441f7ad9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b64bfa138619 (since I'm pretty confident the orange is the other bug)
Reporter | ||
Comment 17•12 years ago
|
||
Issue resolved on a glance tour through a latest inbound; (06/07); YouTube front-page is not overly-inflated as seen in comment #0's attachment-screenshot. Marking status-firefox16 as fixed.
status-firefox16:
--- → fixed
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b64bfa138619
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Assignee | ||
Comment 20•12 years ago
|
||
Need FF14 (beta) approval request and uplift on this one.
Comment on attachment 630756 [details] [diff] [review] revert the overflow part of the phoronix.com fix [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 747720 User impact if declined: inconsistent inflation on youtube.com Testing completed (on m-c, etc.): on mozilla-central Risk to taking this patch (and alternatives if risky): somewhat risky in that it's changing a good bit, but it's reverting towards previous behavior so likely to be largely a known risk. (It's only reverting part of bug 747720, though.) String or UUID changes made by this patch: none
Attachment #630756 -
Flags: approval-mozilla-beta?
Attachment #630756 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Attachment #630756 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Backed out of Aurora due to perma-orange on Linux PGO Moth. https://hg.mozilla.org/releases/mozilla-aurora/rev/5838fc84511b
Relanded: https://hg.mozilla.org/releases/mozilla-aurora/rev/dd1621e5f855 and I think it's good, despite that I've relanded all 3 patches unchanged.
Updated•12 years ago
|
Keywords: testcase-wanted
Reporter | ||
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Keywords: testcase-wanted
Reporter | ||
Updated•12 years ago
|
Keywords: testcase-wanted
Updated•12 years ago
|
Attachment #630756 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 26•12 years ago
|
||
Going to back out this change and go for moz-document hack if we can't evangelize to you tube.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Phoronix fix relanded: https://hg.mozilla.org/mozilla-central/rev/880037c0ff1e https://hg.mozilla.org/releases/mozilla-aurora/rev/bbbcb681820d https://hg.mozilla.org/releases/mozilla-beta/rev/494a2756d358
In case we need it (but I hope we don't), a patch and test build to special-case www.youtube.com's front page are: https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/35ffb1d60b53/disable-inflation-youtube https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbaron@mozilla.com-efed845689d9/try-android/fennec-16.0a1.en-US.android-arm.apk
(In reply to David Baron [:dbaron] from comment #28) > In case we need it (but I hope we don't), a patch and test build to > special-case www.youtube.com's front page are: > > https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ > 35ffb1d60b53/disable-inflation-youtube > > https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbaron@mozilla. > com-efed845689d9/try-android/fennec-16.0a1.en-US.android-arm.apk And this build is the above patch on top of https://hg.mozilla.org/mozilla-central/rev/d39501066aa4
Comment 30•12 years ago
|
||
This is still an issue on Firefox Mobile Native 14 beta 7.
Reporter | ||
Comment 31•12 years ago
|
||
(In reply to adrian tamas from comment #30) > This is still an issue on Firefox Mobile Native 14 beta 7. That is because the Phoronix fix relanded in comment #27 due to some other site issues, and comment #26 has the two potential paths to correction
Assignee | ||
Comment 32•12 years ago
|
||
I got the following response from John Harding, Director of Engineering, YouTube:
> Thanks for reaching out - I'll have our team add your UA to our list of mobile UAs we redirect to m.youtube.com.
Since we don't have these issues on m.youtube.com, let's close this bug as RESOLVED after we verify the server-side redirect.
Comment 33•12 years ago
|
||
Will m.youtube.com feed us flash or h264 <video>? If it's the latter, we will be in a worse spot.
Comment 34•12 years ago
|
||
We use to get kicked to the Youtube app in XUL Fennec.
Assignee | ||
Comment 35•12 years ago
|
||
(In reply to Joe Drew (:JOEDREW!) from comment #33) > Will m.youtube.com feed us flash or h264 <video>? If it's the latter, we > will be in a worse spot. We've already tested m.youtube.com on Fennec and it works well and looks nicer than www.youtube.com.
Comment 36•12 years ago
|
||
Ah, it seems to kick us to the desktop site when you click play. WFM.
Updated•12 years ago
|
Whiteboard: [readability] → [readability][special case patch in comment 28]
Comment 37•12 years ago
|
||
qawanted to pre-qualify dbaron's work around patch in comment 28
Keywords: qawanted
Whiteboard: [readability][special case patch in comment 28] → [readability]
Updated•12 years ago
|
Whiteboard: [readability] → [readability][special case patch in comment 28]
Updated•12 years ago
|
Whiteboard: [readability][special case patch in comment 28] → [readability][anticipate youtube fix wednesday][special case patch in comment 28]
Updated•12 years ago
|
Whiteboard: [readability][anticipate youtube fix wednesday][special case patch in comment 28] → [readability][anticipate youtube fix wednesday][special case patch in comment 28][IfWeRespin14_0]
Reporter | ||
Comment 38•12 years ago
|
||
As of 6:00 EDT/3:00 PDT, YouTube is serving us mobile now.
Comment 39•12 years ago
|
||
(In reply to Aaron Train [:aaronmt] from comment #38) > As of 6:00 EDT/3:00 PDT, YouTube is serving us mobile now. The mobile page yes, although when you play videos, you bounce out to the desktop site.
Comment 40•12 years ago
|
||
True, but I suspect we can get it off the blocker list at least now.
As I understood it, the problem in this bug affects only the homepage and not the video pages, which I think means this isn't a problem anymore.
Comment 42•12 years ago
|
||
(In reply to David Baron [:dbaron] from comment #41) > As I understood it, the problem in this bug affects only the homepage and > not the video pages, which I think means this isn't a problem anymore. Gotcha. Makes sense. The desktop site issue when viewing a video is a separate bug as I understand it anyways as well (bug 653833).
OK, marking fixed by youtube's change, and moving to a component appropriate for such a marking.
Assignee: dbaron → bugs
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Component: Layout → English US
Flags: approval-mozilla-beta+
Flags: approval-mozilla-aurora+
Product: Core → Tech Evangelism
QA Contact: general → english-us
Resolution: --- → FIXED
Target Milestone: mozilla16 → ---
Version: Trunk → unspecified
Reporter | ||
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Keywords: qawanted
Whiteboard: [readability][anticipate youtube fix wednesday][special case patch in comment 28][IfWeRespin14_0] → [readability][special case patch in comment 28][IfWeRespin14_0]
Updated•12 years ago
|
Whiteboard: [readability][special case patch in comment 28][IfWeRespin14_0] → [readability][special case patch in comment 28]
Brad asked me to get review on this in case we need to use it.
Attachment #636597 -
Flags: review?(bzbarsky)
Comment 45•12 years ago
|
||
Comment on attachment 636597 [details] [diff] [review] hack for youtube This eems like it should use url-prefix() or just domain(), no?
Comment 46•12 years ago
|
||
s/eems/seems/
No, because it's really intended to change only the front page and not any of the subpages.
Comment 48•12 years ago
|
||
Comment on attachment 636597 [details] [diff] [review] hack for youtube Oh. People use the front page? ;) In that case, and explicit comment to that effect is a good idea.
Attachment #636597 -
Flags: review?(bzbarsky) → review+
Updated•9 years ago
|
Product: Tech Evangelism → Tech Evangelism Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•