300.09 KB, image/png
47 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
1.47 MB, application/zip
76.83 KB, application/zip
Created attachment 8600594 [details] ReaderFirefoxSafariMaxFontSizeComparison.png User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/600.5.17 (KHTML, like Gecko) Version/8.0.5 Safari/600.5.17 Steps to reproduce: 1. Open a webpage having Reader Mode available (e.g. http://en.wikipedia.org/wiki/Firefox ) 2. Launch Reader Mode 3. Tap the display settings button and choose maximum font size 4. Tap outsize of the display settings popover to look at the webpage Actual results: The size is not sufficient for people with need for big letters (low-vision people, includin also simply many older people). Expected results: The text size is sufficient - see attached comparison of maximum size in Safari's Reader (with big enough letters) and Firefox's Reader Mode. Safari's text size is such that any bigger would probably mean words would not fit in screen width, so it is probably the maximum reasonable size (or very close to it).
From brief look, this would probably just require adding new cases to the ReaderModeFontSize enum, and adjust the Reader mode's CSS to include styles for these (i.e. after the maximum .font-size5, have also .font-size6, .font-size7 etc.).
Created attachment 8601190 [details] [review] Pull request
Darrin/Robin, what do you think of these new font sizes?
I think all we would need to do here is expand how many steps we can are utilizing. Currently we are only using 4 (total), while Safari has 11 (3 down, 8 up from default). My suggestion: expand steps to 12: 4 down, 8 up.
9 up would be one more than Safari. Wouldn't that make us better?
:wesj actually, I added 1 extra step to the smaller size, so yeah, we're one-upping already.
Comment on attachment 8601190 [details] [review] Pull request Looks good, but I think we want one more size as mentioned in comment 4.
Robin, I am not sure how to exactly implement this. In the pull request, I am using these sizes in pixels for main body text (let's focus on those for main body text as they are the most important, I will be able to adjust the other 2 types of sizes based on the answers to questions below): size1: 14px size2: 16px size3: 18px size4: 20px size5: 22px size6: 25px size7: 28px size8: 31px size9: 35px size10: 40px size11: 45px size12: 50px The size1 to size5 were already there before the PR. I left the default to be size3. So now we have one default size (size3), 2 steps down (to size1), 9 steps up (to size12). I was thinking adding one size smaller than size1 (e.g. size0), having size of 12px or 13px, and moving the default size to be size4 instead of size3. This would make us have the required number of steps up and down. It would make the default font size ~10% bigger though (from 18px to 20px). So I was wondering if you are OK with this, or have some other suggestion on how to achieve the suggested number of sizes/steps. Thanks.
The default body text is spec'd to 16px (1em), so how does the below work? (body copy sizes only, we can also figure out what the h1 sizes are) 12px 0.750em 13px 0.813em 14px 0.875em 15px 0.938em 16px 1.000em (baseline) 17px 1.063em 18px 1.125em 19px 1.188em 20px 1.250em 21px 1.313em 22px 1.375em 23px 1.438em 24px 1.500em
Boris, where are we at with this?
Hi Robin, apologies for being unresponsive lately. I will finally get to a response during the next 2 days.
Hello Robin, the sizes you proposed, that is 13 levels (1 default + 4 smaller + 8 larger) ranging 12px - 24px in body font size, are in my opinion insufficient w.r.t maximum size. I have no problem with the number of levels or the smallest size, but the biggest size is IMHO too small. You are proposing maximum body (.content) size of 24px, in the pull request I originally proposed 45px (i.e. roughly twice as bigger) as that is comparable to the maximum Reader body font sizes Safari uses (and which is beneficial to people who have some more serious visual impairment, which was my primary motivation for this, but I could imagine this might be useful even during some presentations of content to public). I updated the pull request attached to this bug to have new code that: - is rebased on latest master - reflects the total number of 13 size levels you suggested (as opposed to 12 I originally proposed), including 4 steps down / 8 steps up from the default - respects the 16px default body font size - stipulates a 48px maximum body size (slightly bigger than Safari's maximum, and is roughly the threshold that with any bigger size, it would no longer be an exception that one word does not fit on the width of a screen of iPhone in portrait mode) - stipulates geometric series (i.e. each next value is the same multiple of the previous one); specifically with size5 being 16px and size13 being 48px, I got that the size of each level should be ~14.7% more than the previous level. I think this "feels" more natural than increasing linearly (i.e. by a fixed number of pixels each level), but I could be wrong. The results (after some adjustment for 2 smallest sizes, and one overriding of rounding in size12) for the body size are: size1: 10px size2: 11px size3: 12px size4: 14px size5: 16px size6: 18px size7: 21px size8: 24px size9: 28px size10: 32px size11: 37px size12: 42px size13: 48px I am attaching a screenshot of each size (source: https://en.wikipedia.org/wiki/Serena_Williams) I am also attaching a Numbers table where I calculated the sizes (you can modify the "Anchors" table, i.e. which 2 levels have fixed values and which are those, and the "Body sizes" table recalculates). I also left there values for hypothetical levels beyond 13 (see note about iPad below) for an idea how the sizes could evolve, or to enable experimenting with smaller changes between each level (i.e. to anchor size5 @ 16px, and size17 @ 48px). (both attachments will be in follow-up comments, as they can't be attached to this comment directly) Could you please tell me what do you think about these body sizes? Would you like me to use different sizes? I should also note that Safari makes the last 2 steps a big increase (i.e. while other steps increase by approx. 1-2 px, the last 2 steps both increase by 8px. But I find no reason why we would want to emulate that behavior as well. One last thing: I think it would be proper to have even bigger maximum size on iPad (to use the larger screen estate), but I propose not to complicate things with it now and to leave it for a follow-up bug. I am just mentioning it in case you would like to somehow consider it already now. I have modified the other 2 size types (1 for headings, 1 for domain/caption/credits), but only to the extent to bring them up to date with your recent changes (i.e. to make size6 of "h1" bigger than the new size5). I think that, as you suggested, it's better we work out the body sizes first and then specify the other 2. Looking forward to your feedback, and once again sorry for the delay in my response.
Created attachment 8622106 [details] Reader View body sizes screenshots Screenshot of Reader View's body size for each of the 13 sizes. (source page: https://en.wikipedia.org/wiki/Serena_Williams)
Created attachment 8622107 [details] Reader View body size calculation Numbers table I used to calucate sizes of Reader View's body font, and that can be used to experiment with them. Modify entries in the "Anchors" table, and the sizes in "Body" table update.
I think the table you worked out is great. I did only intend for 12 steps, but see now how it became 13. The rest look good and scale nicely. I am working on the CSS for iPad right now, and would like to take a look at this on iPad. Do you have a branch I can test with? Thank for you for your work on this!
(In reply to Robin Andersen [:tecgirl] from comment #16) > Do you have a branch I can test with? Yes, branch accessibility-bug1160756-readermode-bigger in my fork of firefox-ios on github (i.e. dusek/firefox-ios): https://github.com/dusek/firefox-ios/tree/accessibility-bug1160756-readermode-bigger Also there is a pull request for exactly this branch: https://github.com/mozilla/firefox-ios/pull/419
Also on iPad, I think one way to approach it would be that we could simply expand the number of sizes (i.e. add size14, size15 up to size21 as suggested above) (simply add them to the ReaderModeFontSize enum and then define them in the CSS), and have the Swift code return different value for ReaderModeFontSize.isLargest (it could return Size13 on iPhone and Size21 on iPad).
Over to st3fan for landing. Robin, chime in if anything more is needed from you!
I have merged this. If iPad specific changes are wanted then we can do that as part of bug 1173885