Closed Bug 760098 Opened 8 years ago Closed 7 years ago

Default (minTwips; 120) text-inflation on YouTube makes us look bad

Categories

(Tech Evangelism Graveyard :: English US, defect, P1)

ARM
Android
defect

Tracking

(blocking-kilimanjaro:+, firefox14 verified, firefox15+ verified, firefox16 verified, blocking-fennec1.0 +)

VERIFIED FIXED
blocking-kilimanjaro +
Tracking Status
firefox14 --- verified
firefox15 + verified
firefox16 --- verified
blocking-fennec1.0 --- +

People

(Reporter: aaronmt, Assigned: bugs)

References

(Blocks 1 open bug, )

Details

(Keywords: regression, Whiteboard: [readability][special case patch in comment 28])

Attachments

(3 files)

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
Blocks: youtube.com
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.
Component: General → Layout
Product: Fennec Native → Core
QA Contact: general → layout
Version: unspecified → Trunk
No longer blocks: 747720
QA Contact: layout → general
Keywords: testcase-wanted
Blocks: 747720
blocking-kilimanjaro: --- → ?
blocking-fennec1.0: ? → +
To David for a look. Regression from bug 747720.
Assignee: nobody → dbaron
Whiteboard: [readability]
probably need to back out last patch (overflow) from bug 747720
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)
There was a talk of a white or black list.
No, I thought of a better solution, but now I've forgotten.
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.)
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.)
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.
Duplicate of this bug: 762397
https://hg.mozilla.org/mozilla-central/rev/b64bfa138619
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
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?
Attachment #630756 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Keywords: testcase-wanted
Keywords: testcase-wanted
Attachment #630756 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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 → ---
(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
This is still an issue on Firefox Mobile Native 14 beta 7.
(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
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.
Will m.youtube.com feed us flash or h264 <video>? If it's the latter, we will be in a worse spot.
We use to get kicked to the Youtube app in XUL Fennec.
(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.
Ah, it seems to kick us to the desktop site when you click play. WFM.
Whiteboard: [readability] → [readability][special case patch in comment 28]
qawanted to pre-qualify dbaron's work around patch in comment 28
Keywords: qawanted
Whiteboard: [readability][special case patch in comment 28] → [readability]
Whiteboard: [readability] → [readability][special case patch in comment 28]
Whiteboard: [readability][special case patch in comment 28] → [readability][anticipate youtube fix wednesday][special case patch in comment 28]
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]
As of 6:00 EDT/3:00 PDT, YouTube is serving us mobile now.
(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.
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.
(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: 8 years ago7 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
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]
Whiteboard: [readability][special case patch in comment 28][IfWeRespin14_0] → [readability][special case patch in comment 28]
Attached patch hack for youtubeSplinter Review
Brad asked me to get review on this in case we need to use it.
Attachment #636597 - Flags: review?(bzbarsky)
Comment on attachment 636597 [details] [diff] [review]
hack for youtube

This eems like it should use url-prefix() or just domain(), no?
No, because it's really intended to change only the front page and not any of the subpages.
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+
Duplicate of this bug: 771096
Duplicate of this bug: 771096
Product: Tech Evangelism → Tech Evangelism Graveyard
You need to log in before you can comment on or make changes to this bug.