1399616 - WordPress emoji are enormous in Reader View

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect, P3)

Description

[Taylor Hunt]

Attachment: 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?

Comment 1

[Sorina Florean [:sflorean]]

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.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Happens on desktop as well.

Comment 3

[:Gijs (he/him)]

(In reply to Kevin Brosnan [:kbrosnan] from comment #2)
> Happens on desktop as well.

Uh, does it? I can't reproduce on desktop.

Comment 4

[:Gijs (he/him)]

(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)

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

Attachment: bug1399616.png

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

I can reproduce this as well, on Windows 10 Nightly 58.0a1 (2017-10-30) (64-bit)

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 10

[:Gijs (he/him)]

(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. :-(

Comment 11

[Taylor Hunt]

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>

Comment 12

[:Gijs (he/him)]

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.

Comment 13

[Taylor Hunt]

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.

Comment 14

[:Gijs (he/him)]

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

Comment 15

[:Gijs (he/him)]

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.

Comment 16

[:Gijs (he/him)]

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?

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 19

[Taylor Hunt]

I can still reproduce on my Android, Nightly 62.0a1
(2018-06-18)

Comment 20

[:Gijs (he/him)]

I can reproduce on Android, too. Thanks!

Comment 21

[:Gijs (he/him)]

Attachment: Bug 1399616 - add emoji styling to reader mode,

Review commit: https://reviewboard.mozilla.org/r/253182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253182/

Comment 22

[:Gijs (he/him)]

(and yes we should unify the mobile stylesheet more with the toolkit one, but that's a separate bug...)

Comment 23

[Johann Hofmann [:johannh]]

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.

Comment 24

[:Gijs (he/him)]

Comment on attachment 8987892 [details]
Bug 1399616 - add emoji styling to reader mode,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253182/diff/1-2/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23b5f0e1a106
add emoji styling to reader mode, r=johannh

Comment 26

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/23b5f0e1a106

Comment 27

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/23b5f0e1a106