Closed
Bug 1265304
Opened 8 years ago
Closed 7 years ago
Display estimated reading time in Reader Mode
Categories
(Toolkit :: Reader Mode, enhancement, P5)
Toolkit
Reader Mode
Tracking
()
VERIFIED
FIXED
mozilla53
People
(Reporter: djthrottle, Assigned: fiveNinePlusR)
References
Details
(Whiteboard: [reader-mode-firefox-integration])
Attachments
(3 files, 9 obsolete files)
186.11 KB,
image/png
|
fiveNinePlusR
:
ui-review+
|
Details |
54.48 KB,
image/png
|
fiveNinePlusR
:
ui-review+
|
Details |
17.11 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
It would be cool to have an estimated reading time field on top of an article when read in Reader Mode. For example, articles on Medium (e.g. https://medium.com/the-story/read-time-and-you-bc2048ab620c#.o6uf2ydhh) have an estimated reading time on top. Incidentally, the linked article also explains how they estimate it.
Updated•8 years ago
|
Component: General → Reader Mode
Product: Firefox → Toolkit
Comment 1•8 years ago
|
||
I'd happily take a patch but I don't think we'll prioritize this for working on otherwise.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P5
Updated•8 years ago
|
Whiteboard: [reader-mode-firefox-integration]
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → fiveNinePlusR
Assignee | ||
Comment 2•8 years ago
|
||
How sophisticated should we be with the timing metric? Seems like there is some variability in speed based on languages: http://iovs.arvojournals.org/article.aspx?articleid=2166061
Flags: needinfo?(gijskruitbosch+bugs)
Comment 3•8 years ago
|
||
(In reply to fiveNinePlusR from comment #2) > How sophisticated should we be with the timing metric? Seems like there is > some variability in speed based on languages: > > http://iovs.arvojournals.org/article.aspx?articleid=2166061 We do do some language detection already. It seems like detection based on number of characters could then use the speeds from table 2 in that article for the languages where we have that information, and fall back to English where we do not. I wouldn't trust our ability to detect words in different languages, and I also don't think the value of this feature would be worth the code complexity of writing specific word detection for all languages.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 4•8 years ago
|
||
First pass with just English support... just curious if those areas are where it should be implemented in your opinion. I will see about using that table data for other languages.
Attachment #8804835 -
Flags: feedback?(gijskruitbosch+bugs)
Comment 5•8 years ago
|
||
Comment on attachment 8804835 [details] [diff] [review] bug1265304-reader_mode_time.diff Review of attachment 8804835 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/reader/AboutReader.jsm @@ +742,5 @@ > }, > > + _formatReadTime: function(lowEstimate, highEstimate) { > + if (lowEstimate == highEstimate) { > + return highEstimate + " " + gStrings.GetStringFromName("aboutReader.readEstimateMinute"); You should use something like: gStrings.formatStringFromName("aboutReader.readEstimateMinute", [highEstimate], 1); (the array is an array of replacement values to go into the string, the last value is the length of that array because the API is old and dumb and C++ based) Note that for this to work you'll need to update the string to include a "%S" for where you want the value to go. @@ +745,5 @@ > + if (lowEstimate == highEstimate) { > + return highEstimate + " " + gStrings.GetStringFromName("aboutReader.readEstimateMinute"); > + } > + > + return `${lowEstimate}-${highEstimate} ` + gStrings.GetStringFromName("aboutReader.readEstimateMinute"); You should then probably use a different string here, with an array of 2 parameters and two %S's in the string, because other languages might not use the - to delimit the range, or they might put the low estimate 'after' the high one (e.g. right-to-left languages). ::: toolkit/components/reader/ReaderMode.jsm @@ +447,4 @@ > article.title = Cc["@mozilla.org/parserutils;1"].getService(Ci.nsIParserUtils) > .convertToPlainText(article.title, flags, 0); > > + [article.readTimeHigh, article.readTimeLow] = this._getReadTime(article.content); The destructuring here is quite clever... but I wonder if it would make more sense to rename the _getReadTime helper to _assignReadTime, and to pass it the |article| object, and have the helper assign directly to the article. @@ +504,5 @@ > + * > + * @param length The length in words of the content. > + * @return Array > + */ > + _getReadTime: function (article) { I believe article.content is HTML - you probably need to use the parserutils, like in the line above the callsite, to convert to plaintext before determining the words-based reading time. Nit: we can use ES6 for methods as well, ie: _getReadTime(article) { @@ +507,5 @@ > + */ > + _getReadTime: function (article) { > + const wordsPerMinuteLow = 228 - 30; > + const wordsPerMinuteHigh = 228 + 30; > + const length = article.split(" ").length; I don't think this is an appropriate way of determining the number of words, and it also feels fairly performance-intensive (.split on a long string is not fun) for something so ephemeral. Can we just use the character reading speed instead, and use article.length? @@ +509,5 @@ > + const wordsPerMinuteLow = 228 - 30; > + const wordsPerMinuteHigh = 228 + 30; > + const length = article.split(" ").length; > + const readingTimeSlow = (length / wordsPerMinuteLow).toFixed(); > + const readingTimeFast = (length / wordsPerMinuteHigh).toFixed(); I would prefer using Math.ceil() instead of .toFixed(), so that the result is still a number, and so that we don't end up saying, for very short articles, that this is a "0" minute read. :-) ::: toolkit/components/reader/content/aboutReader.html @@ +19,4 @@ > <a id="reader-domain" class="domain"></a> > <div class="domain-border"></div> > <h1 id="reader-title"></h1> > + <h2 id="reader-estimated-time"></h2> I think this should go underneath the byline, and it should probably be a <span> or <div> instead of an <h2>, as it isn't semantically a header (nor should it be really big)? We should just get a UI/UX review on the next iteration of your patch. :-)
Attachment #8804835 -
Flags: feedback?(gijskruitbosch+bugs) → feedback+
Comment 6•8 years ago
|
||
(Apologies for the delay, by the way - I was away yesterday. I generally try to get back to review requests within 1 working day.)
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8804835 -
Attachment is obsolete: true
Attachment #8815564 -
Flags: ui-review?
Attachment #8815564 -
Flags: feedback?(gijskruitbosch+bugs)
Assignee | ||
Comment 8•7 years ago
|
||
This is a patch that does the language detection in a blocking fashion. Not sure the best approach to do it asynchronously though without a lot of work and extra complexity. Perhaps you have some insight Gijs?
Comment 9•7 years ago
|
||
Comment on attachment 8815564 [details] [diff] [review] bug1265304-patch.diff Review of attachment 8815564 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good. Bug 1318605 will add a language detection promise already, so we can piggyback on the code it adds for the next version. I expect it'll land again either today or tomorrow. I'll do a trypush in a second for the ui review and ping someone specific. The next patch (based on bug 1318605) can ask for review? instead of feedback. :-) ::: toolkit/components/reader/ReaderMode.jsm @@ +532,5 @@ > + const charactersPerMinuteHigh = readingSpeed.cpm + readingSpeed.variance; > + const length = article.length; > + > + article.readingTimeSlow = Math.ceil(length / charactersPerMinuteLow); > + article.readingTimeFast = Math.ceil(length / charactersPerMinuteHigh); Nit: these variable names should probably include the unit. Maybe "readingTimeMinsSlow" and "readingTimeMinsFast" or something. @@ +540,5 @@ > + /** > + * Returns the reading speed of a selection of languages with likely variance. > + * > + * @return object with characters per minute and variance. Defaults to English > + * if no suitable language is found in the collection. The comment should probably explain where the data is sourced from. @@ +560,5 @@ > + [ "ru", {cpm: 986, variance: 175 } ], > + [ "sk", {cpm: 885, variance: 145 } ], > + [ "es", {cpm: 1025, variance: 127 } ], > + [ "sv", {cpm: 917, variance: 156 } ], > + [ "tr", {cpm: 1054, variance: 156 } ] Nit: please sort the list alphabetically, maybe with english specialcased at the top (or maybe not - I'm not 100% sure). Nit: please add a trailing comma at the end.
Attachment #8815564 -
Flags: feedback?(gijskruitbosch+bugs) → feedback+
Comment 10•7 years ago
|
||
Comment on attachment 8815564 [details] [diff] [review] bug1265304-patch.diff Blake, can you ui-review the design / look-and-feel of this addition?
Attachment #8815564 -
Flags: ui-review? → ui-review?(bwinton)
Comment 11•7 years ago
|
||
(I'm going to redirect this to someone else, but we're still deciding who. ;)
Comment 12•7 years ago
|
||
No matter whom, a screenshot would speed things up!
Assignee | ||
Comment 13•7 years ago
|
||
I just noticed that there seems to be a regression in readability.js for the attribution byline. But this screenshot is for the estimated time UI approval and my feature didn't cause that.
Attachment #8815826 -
Flags: ui-review?
Assignee | ||
Comment 14•7 years ago
|
||
Comment 15•7 years ago
|
||
Thank you for the screenshot. I have the feeling that we are missing a bit of hierarchy here. My suggestion here is to create a meta-tags area where the date and the reading time can live happily ever after, with no competition with the author name. What I did: - reduce the margin-bottom on .credits (suggestion: 10px); - create a new div for meta-data with a smaller font-size (suggestion: 0.65em) and a 15px margin-bottom; - add a <hr> element for a stronger separation with the actual content.
Comment 16•7 years ago
|
||
We could even throw a little clock icon in there!
Comment 17•7 years ago
|
||
Comment on attachment 8815564 [details] [diff] [review] bug1265304-patch.diff Redirecting to Emanuela… :)
Attachment #8815564 -
Flags: ui-review?(bwinton) → ui-review?(emanuela)
Comment 18•7 years ago
|
||
Comment on attachment 8815826 [details]
reading time screen shot for ux review.
Redirecting to Emanuela… :)
Attachment #8815826 -
Flags: ui-review? → ui-review?(emanuela)
Assignee | ||
Comment 19•7 years ago
|
||
Looking at the patch for bug 1318605 it brings the language detection outside of the ReaderMode.jsm and into the display controller AboutReader.jsm. Would it be acceptable to put the language detection, text direction, and reading time calculation into the _readerParse area of ReaderMode.jsm? I think there might be a race condition using promises inside the display controller(AboutReader) and I think it makes sense to do the article processing in the ReaderMode controller because that is where the raw HTML gets transformed into a readability article with all the associated attributes. If loading time is a concern we could send events to the display controller when the language is detected but I imagine that a synchronous solution where we do not return the article object until the reading direction and estimated time to read it (similar approach to the above patch) are attached to the article object would be sufficiently fast. Is telemetry data collected for this feature to confirm this?
Flags: needinfo?(gijskruitbosch+bugs)
Comment 20•7 years ago
|
||
(In reply to fiveNinePlusR from comment #19) > Looking at the patch for bug 1318605 it brings the language detection > outside of the ReaderMode.jsm and into the display controller > AboutReader.jsm. Would it be acceptable to put the language detection, text > direction, and reading time calculation into the _readerParse area of > ReaderMode.jsm? We could potentially do that, though we'd need to keep the checks to avoid doing this on Android. > I think there might be a race condition using promises > inside the display controller(AboutReader) Can you clarify what kind of race conditions you're worried about? > and I think it makes sense to do > the article processing in the ReaderMode controller because that is where > the raw HTML gets transformed into a readability article with all the > associated attributes. This seems like a fair point, yes. > If loading time is a concern we could send events to the display controller > when the language is detected but I imagine that a synchronous solution > where we do not return the article object until the reading direction and > estimated time to read it (similar approach to the above patch) are attached > to the article object would be sufficiently fast. That's certainly possible. Have you checked the speed of the language detector? I don't really know how fast/slow it is... > Is telemetry data > collected for this feature to confirm this? There used to be, but not anymore. All telemetry probes expire after a while to avoid us collecting loads of data unnecessarily. That's what happened to that probe, and then we removed the code for it as well... TBH, I would be OK with some limited local testing if the additional checks don't take significantly more time. On Android, where CPU resources are more limited, we won't be doing this anyway.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 21•7 years ago
|
||
> That's certainly possible. Have you checked the speed of the language detector? I don't really know how fast/slow it is...
I ran a few tests on my local machine (early 2011 MBP, 2.3GHz i7, 16GB RAM) and it seemed to consistently perform at the 20-150ms for language detection on the largest pages I could find.
205,177 characters @ 113ms 2:25:00 read time
10k @ 51ms 0:10:00 read time
55k @ 158ms ~0:45:00 read time
19k @ 19ms,
18k @ 22ms,
127k @ 115ms load time 1:25:00 read time
Language detection speed seem to be only mildly related to page size in characters.
Flags: needinfo?(gijskruitbosch+bugs)
Comment 22•7 years ago
|
||
(In reply to fiveNinePlusR from comment #21) > > That's certainly possible. Have you checked the speed of the language detector? I don't really know how fast/slow it is... > > I ran a few tests on my local machine (early 2011 MBP, 2.3GHz i7, 16GB RAM) > and it seemed to consistently perform at the 20-150ms for language detection > on the largest pages I could find. > > 205,177 characters @ 113ms 2:25:00 read time > 10k @ 51ms 0:10:00 read time > 55k @ 158ms ~0:45:00 read time > 19k @ 19ms, > 18k @ 22ms, > 127k @ 115ms load time 1:25:00 read time > > Language detection speed seem to be only mildly related to page size in > characters. OK, this seems like we should be OK then. Thanks for checking!
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 23•7 years ago
|
||
Attachment #8815564 -
Attachment is obsolete: true
Attachment #8815826 -
Attachment is obsolete: true
Attachment #8815829 -
Attachment is obsolete: true
Attachment #8815841 -
Attachment is obsolete: true
Attachment #8815564 -
Flags: ui-review?(emanuela)
Attachment #8815826 -
Flags: ui-review?(emanuela)
Attachment #8817096 -
Flags: ui-review?(emanuela)
Attachment #8817096 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 24•7 years ago
|
||
Comment 25•7 years ago
|
||
Comment on attachment 8817096 [details] [diff] [review] bug1265304-1.diff Review of attachment 8817096 [details] [diff] [review]: ----------------------------------------------------------------- Code-wise this looks pretty good to me - thanks! ::: toolkit/components/reader/ReaderMode.jsm @@ +534,5 @@ > + }, > + > + _maybeAssignTextDirection(article) { > + // TODO: Remove the hardcoded language codes below once bug 1320265 is resolved. > + if (["ar", "fa", "he", "ug", "ur"].includes(this._foundLanguage)) { This should check we don't already have an article.dir set. @@ +587,5 @@ > + ]); > + > + return readingSpeed.get(lang) || readingSpeed.get("en"); > + }, > + Nit: no trailing blank line
Attachment #8817096 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 26•7 years ago
|
||
Attachment #8817096 -
Attachment is obsolete: true
Attachment #8817096 -
Flags: ui-review?(emanuela)
Attachment #8817273 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8817098 -
Flags: ui-review?(emanuela)
Comment 27•7 years ago
|
||
Comment on attachment 8817098 [details]
Screenshot for UI Review
It looks good for me!
Assignee | ||
Comment 28•7 years ago
|
||
Adds a few tests, respects the browser locale for displaying the reading time estimate, and fixes a few things from the last review. https://treeherder.mozilla.org/#/jobs?repo=try&revision=150452010e0fb48bfb878523792fb33ad63a8b97
Attachment #8817273 -
Attachment is obsolete: true
Attachment #8817520 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 29•7 years ago
|
||
The language that the browser is in could be in a LTR language which is where the string translation would be pulled from even though the content of the page is RTL with a different language. We want to align the text with the content but keep the estimated time correctly oriented for the display language direction.
Attachment #8817533 -
Flags: ui-review?(emanuela)
Comment 30•7 years ago
|
||
Comment on attachment 8817520 [details] [diff] [review] bug1265304-3.diff Review of attachment 8817520 [details] [diff] [review]: ----------------------------------------------------------------- This looks good! Minor nits below, then this can land. ::: toolkit/components/reader/AboutReader.jsm @@ +21,5 @@ > var gStrings = Services.strings.createBundle("chrome://global/locale/aboutReader.properties"); > > const gIsFirefoxDesktop = Services.appinfo.ID == "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}"; > +var chromeReg = Components.classes["@mozilla.org/chrome/chrome-registry;1"]. > + getService(Components.interfaces.nsIXULChromeRegistry); You can use: XPCOMUtils.defineLazyServiceGetter(this, "gChromeRegistry", "@mozilla.org/chrome/chrome-registry;1", Ci.nsIXULChromeRegistry); ::: toolkit/components/reader/ReaderMode.jsm @@ +567,5 @@ > + */ > + _getReadingSpeedForLanguage(lang) { > + const readingSpeed = new Map([ > + [ "en", {cpm: 987, variance: 118 } ], > + [ "ar", {cpm: 612, variance: 88 } ], Nit: odd spacing here. ::: toolkit/components/reader/test/browser_readerMode_readingTime.js @@ +18,5 @@ > + yield pageShownPromise; > + yield ContentTask.spawn(browser, null, function* () { > + // make sure there is a reading time on the page and that it displays the correct information > + let readingTimeElement = content.document.getElementById("reader-estimated-time"); > + ok(readingTimeElement !== null, "Reading time element should be in document"); Nit: ok(readingTimeElement, "Reading time element should be in document"); @@ +19,5 @@ > + yield ContentTask.spawn(browser, null, function* () { > + // make sure there is a reading time on the page and that it displays the correct information > + let readingTimeElement = content.document.getElementById("reader-estimated-time"); > + ok(readingTimeElement !== null, "Reading time element should be in document"); > + ok(readingTimeElement.innerText === "9-12 min", "Reading time should be '9-12 min'"); is(readingTimeElement.textContent, "9-12 min", ...); @@ +38,5 @@ > + yield ContentTask.spawn(browser, null, function* () { > + // make sure there is a reading time on the page and that it displays the correct information > + let readingTimeElement = content.document.getElementById("reader-estimated-time"); > + ok(readingTimeElement !== null, "Reading time element should be in document"); > + ok(readingTimeElement.innerText === "1 min", "Reading time should be '1 min'"); Same nits as above.
Attachment #8817520 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 31•7 years ago
|
||
Attachment #8817520 -
Attachment is obsolete: true
Attachment #8817832 -
Flags: review+
Comment 32•7 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a03728016ff Adds an estimated time range to reader mode, r=gijs,ui-r=emanuela
Comment 33•7 years ago
|
||
Sorry, had to back this out for failing browser_narrate.js: https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7b1039e20c9dbf75028d57156c646597c9293f Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9a03728016ff7b76c4850375303e84cce00e4ac4 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40468030&repo=mozilla-inbound [task 2016-12-11T16:48:08.859988Z] 16:48:08 INFO - Entering test bound testNarrate [task 2016-12-11T16:48:08.861880Z] 16:48:08 INFO - Buffered messages logged at 16:48:01 [task 2016-12-11T16:48:08.864576Z] 16:48:08 INFO - Console message: [JavaScript Error: "Error requesting favicon URL for about:reader content: favicon not found for uri" {file: "resource://app/modules/ReaderParent.jsm" line: 66}] [task 2016-12-11T16:48:08.866170Z] 16:48:08 INFO - onRejection@resource://app/modules/ReaderParent.jsm:66:13 [task 2016-12-11T16:48:08.868504Z] 16:48:08 INFO - process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:935:21 [task 2016-12-11T16:48:08.871425Z] 16:48:08 INFO - walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7 [task 2016-12-11T16:48:08.873145Z] 16:48:08 INFO - Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11 [task 2016-12-11T16:48:08.874972Z] 16:48:08 INFO - schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7 [task 2016-12-11T16:48:08.880913Z] 16:48:08 INFO - completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7 [task 2016-12-11T16:48:08.883431Z] 16:48:08 INFO - promiseFaviconLinkUrl/<@resource://gre/modules/PlacesUtils.jsm:1678:9 [task 2016-12-11T16:48:08.884711Z] 16:48:08 INFO - [task 2016-12-11T16:48:08.888024Z] 16:48:08 INFO - Buffered messages finished [task 2016-12-11T16:48:08.889866Z] 16:48:08 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/narrate/test/browser_narrate.js | Uncaught exception - - timed out after 50 tries. Please fix the issue and submit an updated patch. Thank you.
Flags: needinfo?(fiveNinePlusR)
Assignee | ||
Comment 34•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95972e98861004a8e82aa630b82f5dca161cccb1 https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb422086d820273ecc93b632fa0a726672ca12bc Not sure those treeherder jobs do enough tests but it should pass now as the tests pass locally after these changes.
Attachment #8817832 -
Attachment is obsolete: true
Flags: needinfo?(fiveNinePlusR)
Attachment #8817937 -
Flags: review?(gijskruitbosch+bugs)
Comment 35•7 years ago
|
||
Comment on attachment 8817937 [details] [diff] [review] bug1265304-5.diff Review of attachment 8817937 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense! I'll land this again with the nit below addressed... Note that if this sticks this time, we should file a followup bug to get rid of the promise-based (this._foundLanguage()) infrastructure in AboutReader.jsm for the article language entirely, as the information is now guaranteed to be present already... ::: toolkit/components/reader/test/browser_readerMode_readingTime.js @@ +19,5 @@ > + yield ContentTask.spawn(browser, null, function* () { > + // make sure there is a reading time on the page and that it displays the correct information > + let readingTimeElement = content.document.getElementById("reader-estimated-time"); > + ok(readingTimeElement, "Reading time element should be in document"); > + ok(readingTimeElement.textContent === "9-12 min", "Reading time should be '9-12 min'"); You missed the same nit as for the last patch (which I fixed on checkin at the time), ie to use is() to compare these things - so rather than ok(foo == bar), use is(foo, bar), because that will ensure the test framework prints the expected and actual value (gives more information about what's going wrong if the test fails).
Attachment #8817937 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 36•7 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/81a0bc9f0fcd Add an estimated time range to reader mode, r=gijs,ui-r=emanuela
Comment 37•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/81a0bc9f0fcd
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 38•7 years ago
|
||
> Note that if this sticks this time, we should file a followup bug to get rid of the promise-based (this._foundLanguage()) infrastructure in AboutReader.jsm for the article language entirely, as the information is now guaranteed to be present already... I can take care of that. > You missed the same nit as for the last patch Not sure why I didn't notice that change. Thanks for taking care of that!
Comment 39•7 years ago
|
||
As a general reference for the future, it would be nice to add a localization note when there are parameters involved (in particular aboutReader.estimatedReadTimeRange). It's also potentially confusing (min -> minimum, minutes?) According to CLDR, the approach without plural form should be safe, but en-US should use 'min.' http://www.unicode.org/cldr/charts/30/by_type/date_&_time.fields.html#16faa1db9d3012d Maybe a native speaker could confirm that.
Comment 40•7 years ago
|
||
The abbreviation here doesn't seem necessary and we have the space to use the full "minutes" so we should use "minutes" instead. It will also remove the ambiguity. fiveNinePlusR, can you file a follow-up to use "minutes" and add a localization note?
Flags: needinfo?(fiveNinePlusR)
Comment 41•7 years ago
|
||
If we use minutes we'll need to use a proper plural form for "%S minutes", while I'm not completely sure what we can do with "%S-%S minutes". Personally I would stick with an abbreviation.
Comment 42•7 years ago
|
||
This was UI-reviewed, so I'm not sure we really need to change much about the contents of the strings in use... but we should probably add localization comments. I filed bug 1323415 to deal with that and any potential l10n changes. fiveNinePlusR, can you file the followup for comment #38 about this._foundLanguage? Thanks!
Assignee | ||
Comment 43•7 years ago
|
||
Gijs, The bug for comment 38 is: bug 1323331 Jared, I'll take care of the localization issues in bug 1323415
Flags: needinfo?(fiveNinePlusR)
Assignee | ||
Updated•7 years ago
|
Attachment #8817098 -
Flags: ui-review?(emanuela) → ui-review+
Assignee | ||
Updated•7 years ago
|
Attachment #8817533 -
Flags: ui-review?(emanuela) → ui-review+
Comment 44•7 years ago
|
||
Release Note Request (optional, but appreciated) [Why is this notable]: Visible improvement to the Reader mode [Affects Firefox for Android]: yes [Suggested wording]: The Reader Mode now displays the estimated reading time for the page [Links (documentation, blog post, etc)]:
relnote-firefox:
--- → ?
Comment 45•7 years ago
|
||
This seems to be a good feature but could cause confusion to others. Please see bug 1334483
Added to Fx53 Aurora release notes.
Updated•7 years ago
|
QA Contact: oana.horvath
Comment 47•7 years ago
|
||
testplan |
I will take this feature as QA and I've created a test plan which you can find here: https://wiki.mozilla.org/QA/Fennec/Estimated_Reading_Time
Comment 48•7 years ago
|
||
This feature was verified on Fennec Aurora 53, based on the test plan located here: https://wiki.mozilla.org/QA/Fennec/Estimated_Reading_Time. All tests have passed, on the following devices: Phones: -Samsung Galaxy Note 4 (Android 5.0.1) -LG G4 (Android 6.0) -Lenovo A536 (Android 4.4.2) Tablets: -Asus Transformers Pad TF300T (Android 4.2.1) -Huawei MediaPad M2 (Android 5.1.1) -Asus ZenPad 8.0 Z380KL (Android 6.0.1) -HTC Nexus 9 (Android 7.1.1)
Status: RESOLVED → VERIFIED
Comment 49•7 years ago
|
||
This feature was verified on 53.0a2 and 53.0b7 across the following platforms: Win 8.1 x64, Ubuntu 14.04 x64, Mac OS X 10.11. More details on testplan: https://wiki.mozilla.org/QA/Estimated_Reading_Time
Comment 50•7 years ago
|
||
Extremely useful feature. Great work!
You need to log in
before you can comment on or make changes to this bug.
Description
•