Closed Bug 1399616 Opened 4 years ago Closed 3 years ago

WordPress emoji are enormous in Reader View

Categories

(Toolkit :: Reader Mode, defect, P3)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox63 --- fixed

People

(Reporter: tigt, Assigned: Gijs)

Details

(Whiteboard: [reader-mode-firefox-integration][reader-mode-readability-algorithm])

Attachments

(3 files)

Attached image giant-emoji.png
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170912100139

Steps to reproduce:

Visit https://blog.shiftyjelly.com/2011/08/02/amazon-app-store-rotten-to-the-core/ and enter Reader View. Scroll down until you find an emoji.


Actual results:

The emoji takes up the entire article width, and most of the screen height. (See attached file for screenshot.)


Expected results:

Probably don’t scale these like regular images. A size of 1em × 1em would probably be good?
Thanks for report!
I was able to reproduce this on all branches with Huawei Honor (Android 5.1.1), Oneplus Two (Android 6.0.1). Marking as new.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Happens on desktop as well.
Has STR: --- → yes
Component: Reader View → Reader Mode
OS: Unspecified → All
Product: Firefox for Android → Toolkit
Hardware: Unspecified → All
(In reply to Kevin Brosnan [:kbrosnan] from comment #2)
> Happens on desktop as well.

Uh, does it? I can't reproduce on desktop.
Flags: needinfo?(kbrosnan)
(In reply to :Gijs from comment #3)
> (In reply to Kevin Brosnan [:kbrosnan] from comment #2)
> > Happens on desktop as well.
> 
> Uh, does it? I can't reproduce on desktop.

(tested on OS X, if that makes a difference)
Attached image bug1399616.png
Flags: needinfo?(kbrosnan)
I can reproduce this as well, on Windows 10 Nightly 58.0a1 (2017-10-30) (64-bit)
The issue is that aboutReaderContent.css has a universal rule that sets `max-width: 100%; height: auto;`. WordPress marks this up as an <img>, not an emoji unicode character.
There is also no width or height explicitly declared for the <img>, and the actual image is https://s0.wp.com/wp-content/mu-plugins/wpcom-smileys/twemoji/2/svg/1f609.svg which is an SVG that ends up displaying as the size of the viewport when viewed directly.
It looks like the server is doing some user-agent detection and sending down an <img> for platforms it doesn't believe have support for rendering Unicode emoji characters. When viewing this page on an OS that the server *does* believe supports emoji, it will respond with the Unicode emoji character and the problem will not reproduce.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> The issue is that aboutReaderContent.css has a universal rule that sets
> `max-width: 100%; height: auto;`. WordPress marks this up as an <img>, not
> an emoji unicode character.

Ugh. I don't know how to fix this, then. We don't really want to stop sizing "some" images, and there's no way to detect in CSS what the intrinsic size of the image is. I guess we could try to do use JS after reader mode finishes loading to read the `naturalHeight` property of each image (if any) and for any that specify width/height < 32 or so, to set a class that unsets these height/width issues on the assumption that they'll be PNG or SVG emoji... but that seems pretty fragile. :-(
WordPress adds a class to them specifically, so could that be used? Their emoji shim includes this CSS:

<style type="text/css">
img.wp-smiley,
img.emoji {
	display: inline !important;
	border: none !important;
	box-shadow: none !important;
	height: 1em !important;
	width: 1em !important;
	margin: 0 .07em !important;
	vertical-align: -0.1em !important;
	background: none !important;
	padding: 0 !important;
}
</style>
I think we strip the classes (or if we don't, we should, in the general case...). It's a bit tricky because having lots of individual hacks like this isn't really the 'point' of reader mode. On the other hand, the result here is really awful and hopefully the number of hacks required here would be 'small', for some meaning of the word. And, of course, if we keep the 'emoji' class then other places that use those classes will be affected. Which is mostly fine, ish, I think...

Is this a base wordpress feature, or some kind of plugin?

To be fair, the natural height thing I suggested in comment #10 seems to be 0 for svgs without viewbox or width/height info, and inferring that we want 1em for all of them is probably even more prone to other issues... really, we want to know about the image's size on the original page, but that's info we don't have. :-(

So yeah, maybe reusing that class would be the best option here.
Whiteboard: [reader-mode-firefox-integration][reader-mode-readability-algorithm]
It's a default WordPress thing -- I think it was added back when emoji support on Windows was iffy. (https://core.trac.wordpress.org/ticket/31242)

Alternatively, I could see about contributing to WordPress to put `width` and `height` on their Twemoji fallbacks.
(In reply to tigt from comment #13)
> It's a default WordPress thing -- I think it was added back when emoji
> support on Windows was iffy. (https://core.trac.wordpress.org/ticket/31242)

Cool, thanks for clarifying.

> Alternatively, I could see about contributing to WordPress to put `width`
> and `height` on their Twemoji fallbacks.

Hm, well, I can't speak for the wordpress folks, but I could imagine they might not be enthusiastic given that the attributes would be in px, and they're currently specifying size in `em` in CSS, and introducing a tight coupling with the font size in use in the context here might not be much fun...
We now strip classes but have a list of classes to keep, so this is now reasonably fixable (post bug 1417837). Self-ni to come back to this.
Flags: needinfo?(gijskruitbosch+bugs)
I wrote a poc patch for this (only a few lines) but I can't reproduce even on my win10 machine. I just get "real" emoji instead of the <img> tags. Can you still reproduce on current nightly, Jared? tigt, do you have another wordpress post where you're seeing this? Or perhaps I need to build android and test on a phone or something?
Flags: needinfo?(tigt)
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
I can reproduce this issue on my Win7 VM in Reader Mode (downloaded from https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/ and installed Firefox release on it).
Flags: needinfo?(jaws)
However... I *cannot* reproduce this issue using the latest Firefox Nightly on my Windows 7 VM. So I think we can call this bug as resolved-worksforme.
I can still reproduce on my Android, Nightly 62.0a1
(2018-06-18)
Flags: needinfo?(tigt)
I can reproduce on Android, too. Thanks!
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(and yes we should unify the mobile stylesheet more with the toolkit one, but that's a separate bug...)
Comment on attachment 8987892 [details]
Bug 1399616 - add emoji styling to reader mode,

https://reviewboard.mozilla.org/r/253182/#review260128

Yeah, this is a hack. Whenever they change that class name it starts to break again. Granted, it's unlikely that will happen and I don't have a better suggestion, so r=me. I also tested this on Windows 8 and it fixes the issue for me.

I would suggest adding a comment pointing to this bug in the code, though.
Attachment #8987892 - Flags: review?(jhofmann) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/23b5f0e1a106
add emoji styling to reader mode, r=johannh
https://hg.mozilla.org/mozilla-central/rev/23b5f0e1a106
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.