It looks like _updateImageMargins is doing something dumb. With this testcase, it sets the images to be 24px wide: https://medium.com/backchannel/first-alan-adler-invented-the-aerobie-now-he-s-created-the-perfect-cup-of-coffee-c5e94ccc538e

/r/5913 - Bug 1146373 - Don't resize reader view images in JS. r=Gijs Pull down this commit: hg pull review -r 8e5afa2c13beed62b35d13fcf29a63473e0c79c3

This patch matches what's specified in mmaslaney's mock-up for desktop: https://projects.invisionapp.com/share/982H8Z1TZ#/screens And it also maintains the current mobile behavior of going full-width if the image is big enough. I talked through the behavior with antlam on IRC, and we decided we should avoid stretching the image at all, so I updated the logic to account for this. I couldn't think of a way to do this purely in CSS, but setting this extra attribute on the images shouldn't affect desktop.

Comment on attachment 8582075 [details] MozReview Request: bz://1146373/margaret https://reviewboard.mozilla.org/r/5911/#review4897 ::: toolkit/themes/windows/global/aboutReader.css (Diff revision 1) > +/* If an image is placed inside one of these blocks, > + there's no need to add margin at the bottom */ > +.content .wp-caption img, > +.content figure img { Doesn't this also apply in all of the cases above, so where it's in a <p>, <code>, ... ? ::: toolkit/components/reader/AboutReader.jsm (Diff revision 1) > - if (!img._originalWidth) > - img._originalWidth = img.offsetWidth; > - > - let imgWidth = img._originalWidth; > - > - // If the image is taking more than half of the screen, just make > + // If the image is large enough, just make it fill edge-to-edge. > + if (img.naturalWidth >= windowWidth) { > + img.setAttribute("full-width", true); > + } else { > + img.removeAttribute("full-width"); > + } The spec says we want to left-align smaller images. Should we add an attribute for this, too? ::: mobile/android/themes/core/aboutReader.css (Diff revision 1) > +.content img[full-width] { I don't think we purge attributes. Do you think we should use something like "moz-readermode-full-width" so we don't accidentally hit page-provided attributes?

(In reply to :Gijs Kruitbosch from comment #3) > Comment on attachment 8582075 [details] > MozReview Request: bz://1146373/margaret > > https://reviewboard.mozilla.org/r/5911/#review4897 > > ::: toolkit/themes/windows/global/aboutReader.css > (Diff revision 1) > > +/* If an image is placed inside one of these blocks, > > + there's no need to add margin at the bottom */ > > +.content .wp-caption img, > > +.content figure img { > > Doesn't this also apply in all of the cases above, so where it's in a <p>, > <code>, ... ? I took this from the mobile CSS without thinking through it thoroughly... the idea is that we don't want to be adding together two margin-bottom: 30px; styles to end up with a doubly large bottom margin (you can see this currently in articles with large images). Are you suggesting that we should make this style apply even more often? Looking a bit more closely, I realize that the mobile styles are a bit different, so we may actually want more selectors here, such as: .content p > img:only-child, .content p > a:only-child > img:only-child, Or maybe alternately we should remove the margin-bottom we're currently setting on .content img, and instead replace it with a rule to apply that margin only if there is content that directly follows it (if that makes any sense). Or maybe just remove that bottom margin altogether? > ::: toolkit/components/reader/AboutReader.jsm > (Diff revision 1) > > - if (!img._originalWidth) > > - img._originalWidth = img.offsetWidth; > > - > > - let imgWidth = img._originalWidth; > > - > > - // If the image is taking more than half of the screen, just make > > + // If the image is large enough, just make it fill edge-to-edge. > > + if (img.naturalWidth >= windowWidth) { > > + img.setAttribute("full-width", true); > > + } else { > > + img.removeAttribute("full-width"); > > + } > > The spec says we want to left-align smaller images. Should we add an > attribute for this, too? Where did you see that? To me it looked like images should be centered, which is what I did with the CSS. > ::: mobile/android/themes/core/aboutReader.css > (Diff revision 1) > > +.content img[full-width] { > > I don't think we purge attributes. Do you think we should use something like > "moz-readermode-full-width" so we don't accidentally hit page-provided > attributes? That sounds like a good idea, I'll update the patch to do that.

(In reply to :Margaret Leibovic from comment #4) > (In reply to :Gijs Kruitbosch from comment #3) > > Comment on attachment 8582075 [details] > > MozReview Request: bz://1146373/margaret > > > > https://reviewboard.mozilla.org/r/5911/#review4897 > > > > ::: toolkit/themes/windows/global/aboutReader.css > > (Diff revision 1) > > > +/* If an image is placed inside one of these blocks, > > > + there's no need to add margin at the bottom */ > > > +.content .wp-caption img, > > > +.content figure img { > > > > Doesn't this also apply in all of the cases above, so where it's in a <p>, > > <code>, ... ? > > I took this from the mobile CSS without thinking through it thoroughly... > the idea is that we don't want to be adding together two margin-bottom: > 30px; styles to end up with a doubly large bottom margin (you can see this > currently in articles with large images). Are you suggesting that we should > make this style apply even more often? > > Looking a bit more closely, I realize that the mobile styles are a bit > different, so we may actually want more selectors here, such as: > > .content p > img:only-child, > .content p > a:only-child > img:only-child, > > Or maybe alternately we should remove the margin-bottom we're currently > setting on .content img, and instead replace it with a rule to apply that > margin only if there is content that directly follows it (if that makes any > sense). Or maybe just remove that bottom margin altogether? You can't select backwards in CSS, and the DOM tree might make it hard to detect this anyway: <p> <code> <img> <!-- <-- wants margin --> </code> </p> <p>Hi.</p> So basically what we want here is for the <img> to have a margin if it's not inside a container (and the last child of that container) that already has a margin. Seems like the current approach for that is fine, we just need to make the list of things where we remove the margin (set it to 0) match the list of tags whose bottom margin we said. That was what my comment tried to ask for (and probably failed). I don't know that there is a simpler way of doing this. > > > ::: toolkit/components/reader/AboutReader.jsm > > (Diff revision 1) > > > - if (!img._originalWidth) > > > - img._originalWidth = img.offsetWidth; > > > - > > > - let imgWidth = img._originalWidth; > > > - > > > - // If the image is taking more than half of the screen, just make > > > + // If the image is large enough, just make it fill edge-to-edge. > > > + if (img.naturalWidth >= windowWidth) { > > > + img.setAttribute("full-width", true); > > > + } else { > > > + img.removeAttribute("full-width"); > > > + } > > > > The spec says we want to left-align smaller images. Should we add an > > attribute for this, too? > > Where did you see that? To me it looked like images should be centered, > which is what I did with the CSS. There's a pink-ish comment on the "Lindsey Vonn Wins World Cup Title in Downhill" article, specifically on the image, that says: "If available, the header image... Images smaller than 300px will be left-aligned" (I'd link to it or copy-paste it or something but I can't figure out how to do that with this thing, it seems to be entirely "stare-at" only (can't even select the text!) except for leaving new comments, which clearly isn't what I want.)

https://reviewboard.mozilla.org/r/5911/#review5099 ::: toolkit/components/reader/AboutReader.jsm (Diff revision 1) > - img._originalWidth = img.offsetWidth; > + if (img.naturalWidth >= windowWidth) { After some more testing and investigation, this doesn't seem to fix our issue either, since we can still end up with an incorrect `naturalWidth` value here if the image isn't fully loaded yet. I think that the `img.width > 0` check below is not an appropriate check for determining whether the image has already loaded.

(In reply to :Gijs Kruitbosch from comment #5) > (In reply to :Margaret Leibovic from comment #4) > > (In reply to :Gijs Kruitbosch from comment #3) > > > Comment on attachment 8582075 [details] > > > MozReview Request: bz://1146373/margaret > > > > > > https://reviewboard.mozilla.org/r/5911/#review4897 > > > > > > ::: toolkit/themes/windows/global/aboutReader.css > > > (Diff revision 1) > > > > +/* If an image is placed inside one of these blocks, > > > > + there's no need to add margin at the bottom */ > > > > +.content .wp-caption img, > > > > +.content figure img { > > > > > > Doesn't this also apply in all of the cases above, so where it's in a <p>, > > > <code>, ... ? > > > > I took this from the mobile CSS without thinking through it thoroughly... > > the idea is that we don't want to be adding together two margin-bottom: > > 30px; styles to end up with a doubly large bottom margin (you can see this > > currently in articles with large images). Are you suggesting that we should > > make this style apply even more often? > > > > Looking a bit more closely, I realize that the mobile styles are a bit > > different, so we may actually want more selectors here, such as: > > > > .content p > img:only-child, > > .content p > a:only-child > img:only-child, > > > > Or maybe alternately we should remove the margin-bottom we're currently > > setting on .content img, and instead replace it with a rule to apply that > > margin only if there is content that directly follows it (if that makes any > > sense). Or maybe just remove that bottom margin altogether? > > You can't select backwards in CSS, and the DOM tree might make it hard to > detect this anyway: > > <p> > <code> > <img> <!-- <-- wants margin --> > </code> > </p> > <p>Hi.</p> > > > So basically what we want here is for the <img> to have a margin if it's not > inside a container (and the last child of that container) that already has a > margin. > > Seems like the current approach for that is fine, we just need to make the > list of things where we remove the margin (set it to 0) match the list of > tags whose bottom margin we said. That was what my comment tried to ask for > (and probably failed). > > I don't know that there is a simpler way of doing this. I think that we should just get rid of the bottom margin on images. I don't think that I totally thought through this selector when I originally wrote it, since it was part of the "we just need some styles for desktop reader content!" bug. It would be useful to have some testcases here to confirm what the HTML output usually looks like, but I feel like I've mostly ever seen these large images appear within parent elements that we already assign a bottom margin. Simply removing this margin would also fix bug 1128916. > > > ::: toolkit/components/reader/AboutReader.jsm > > > (Diff revision 1) > > > > - if (!img._originalWidth) > > > > - img._originalWidth = img.offsetWidth; > > > > - > > > > - let imgWidth = img._originalWidth; > > > > - > > > > - // If the image is taking more than half of the screen, just make > > > > + // If the image is large enough, just make it fill edge-to-edge. > > > > + if (img.naturalWidth >= windowWidth) { > > > > + img.setAttribute("full-width", true); > > > > + } else { > > > > + img.removeAttribute("full-width"); > > > > + } > > > > > > The spec says we want to left-align smaller images. Should we add an > > > attribute for this, too? > > > > Where did you see that? To me it looked like images should be centered, > > which is what I did with the CSS. > > There's a pink-ish comment on the "Lindsey Vonn Wins World Cup Title in > Downhill" article, specifically on the image, that says: > > "If available, the header image... Images smaller than 300px will be > left-aligned" > > (I'd link to it or copy-paste it or something but I can't figure out how to > do that with this thing, it seems to be entirely "stare-at" only (can't even > select the text!) except for leaving new comments, which clearly isn't what > I want.) Thanks for pointing this out, I must have just looked at the pretty pictures without reading the fine print :)

I rebased this on top of another unlanded patch and mozreview got confused, so I'm giving up and attaching a patch.

Comment on attachment 8583460 [details] [diff] [review] Don't resize reader view images in JS Review of attachment 8583460 [details] [diff] [review]: ----------------------------------------------------------------- I mean, I'll take it, but I would note that it seems odd that we might in principle not have a margin now, despite the spec calling for one... Then again, in practice I imagine almost any image will be contained in something anyway...

(In reply to :Gijs Kruitbosch (away 26-30/3) from comment #9) > Comment on attachment 8583460 [details] [diff] [review] > Don't resize reader view images in JS > > Review of attachment 8583460 [details] [diff] [review]: > ----------------------------------------------------------------- > > I mean, I'll take it, but I would note that it seems odd that we might in > principle not have a margin now, despite the spec calling for one... > > Then again, in practice I imagine almost any image will be contained in > something anyway... Yeah, I'm sorry, I know this isn't ideal :( The spec is a bit challenging to implement, given that we're not in control of the mark-up we're getting here. Maybe we can use Readability.js to ensure that images are wrapped in one of these elements. Or maybe there's even already logic that takes care of that? It looks like a lone <img> tag might end up getting wrapped in a <p> in that "experimental" logic block. I can play around with some more testcases before landing this to see if there's a state where this looks broken.

(In reply to :Margaret Leibovic from comment #10) > The spec is a bit challenging to implement, given that we're not in control > of the mark-up we're getting here. Maybe we can use Readability.js to ensure > that images are wrapped in one of these elements. Or maybe there's even > already logic that takes care of that? It looks like a lone <img> tag might > end up getting wrapped in a <p> in that "experimental" logic block. I can > play around with some more testcases before landing this to see if there's a > state where this looks broken. After some more testing, I found that we don't wrap these image elements in a parent element, but I don't have time to continue working with this right now, so I decided to just land what I have: https://hg.mozilla.org/integration/fx-team/rev/47d3cee563e5 I filed bug 1148189 for further investigation.

Comment on attachment 8583460 [details] [diff] [review] Don't resize reader view images in JS Approval Request Comment [Feature/regressing bug #]: reader view [User impact if declined]: images may be too small [Describe test coverage new/current, TreeHerder]: baked on nightly/aurora [Risks and why]: low-risk, added some CSS style changes and removed some manual style setting logic in JS [String/UUID change made/needed]: none

Comment on attachment 8583460 [details] [diff] [review] Don't resize reader view images in JS should be in 38 beta 2

Using the testcase from comment #0 I'm verifying the bug as fixed on: Firefox mobile: 38 Beta 2, Nightly 40.0a1 and Aurora 39.0a2 Firefox desktop: 38 Beta 2, Nightly 40.0a1 and Aurora 39.0a2