Do language detection for cases where we don't obtain a dir attribute

VERIFIED FIXED in Firefox 53

Status

()

Toolkit
Reader Mode
VERIFIED FIXED
9 months ago
5 months ago

People

(Reporter: evanxd, Assigned: evanxd)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 months ago
> for cases where that does not obtain a dir attribute, in the reader page JS/HTML (not in the readability code itself, where we can do the other stuff), abstract the language detection that we're doing for narrate anyway out, and make it more accessible, then use that to determine a fallback rtl/ltr-ness as per comment #13 . [1]

Do language detection for cases don't obtain a dir attribute.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1173548#c21
(Assignee)

Updated

9 months ago
No longer blocks: 1173548
Depends on: 1173548
(Assignee)

Comment 1

9 months ago
Created attachment 8812126 [details] [diff] [review]
WIP Patch

It's the WIP patch. Next step, need to figure out how to determine text direction by language.
(Assignee)

Updated

9 months ago
Attachment #8812126 - Attachment is obsolete: true
(Assignee)

Comment 2

9 months ago
> I mean that we should call LanguageDetector.jsm in reader mode code instead of in narrator code, and the result be saved somewhere where it is accessible to the narrator code - the narrator code can depend on the reader mode code but not vice versa.

Hi Gijs,

According to you said for cases where that does not obtain a dir attribute[1], I have another idea to instead of saving the language detector result somewhere, how about we just add new a param "language" for "NarrateControls.jsm" and "Narrator.jsm" objects? It will looks like,

1. Do language detection in "AboutReader.jsm" and assign the result into the "language" variable
2. new a "NarrateControls" and a "Narrator" objects with `new NarrateControls(mm, win, language)` and `new Narrator(win, language)`

I think the idea could be simple and clear. What do you think?

If we really want to do the saving result in somewhere idea, could you tell me more clues. Honestly, I don't know which place could be good enough one to save the result.

And one more question, how to get text direction of an article by knowing the language detection result? Do we need to do something like
```
LanguageDetector.detectLanguage(text).then(result => {
  let language = result.confident ? result.language : null;
  let dir = null;
  switch (language) {
    case "ar":
    case "he":
    case "fa":
    case "ug":
    case "ur":
    case "syr":
      dir = "rtl";
  }
});

```[2]

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1173548#c24
[2]: http://searchfox.org/mozilla-central/source/browser/base/content/utilityOverlay.js#637-646
Flags: needinfo?(gijskruitbosch+bugs)

Comment 3

9 months ago
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #2)
> > I mean that we should call LanguageDetector.jsm in reader mode code instead of in narrator code, and the result be saved somewhere where it is accessible to the narrator code - the narrator code can depend on the reader mode code but not vice versa.
> 
> Hi Gijs,
> 
> According to you said for cases where that does not obtain a dir
> attribute[1], I have another idea to instead of saving the language detector
> result somewhere, how about we just add new a param "language" for
> "NarrateControls.jsm" and "Narrator.jsm" objects? It will looks like,
> 
> 1. Do language detection in "AboutReader.jsm" and assign the result into the
> "language" variable
> 2. new a "NarrateControls" and a "Narrator" objects with `new
> NarrateControls(mm, win, language)` and `new Narrator(win, language)`
> 
> I think the idea could be simple and clear. What do you think?

There are different ways we could expose the language to the narrate code, and this is one of them. But the language detection code returns a promise, so I don't know if it makes sense to wait for the promise to resolve before calling the narrate code, or if we should pass through the promise (or have the narrate code fetch the promise off a page global or whatever). Do what looks best to you.

> And one more question, how to get text direction of an article by knowing
> the language detection result? Do we need to do something like
> ```
> LanguageDetector.detectLanguage(text).then(result => {
>   let language = result.confident ? result.language : null;
>   let dir = null;
>   switch (language) {
>     case "ar":
>     case "he":
>     case "fa":
>     case "ug":
>     case "ur":
>     case "syr":
>       dir = "rtl";
>   }
> });
> 
> ```[2]
> 
> [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1173548#c24
> [2]:
> http://searchfox.org/mozilla-central/source/browser/base/content/
> utilityOverlay.js#637-646

I don't know. Flod?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(francesco.lodolo)
It doesn't look like a great idea to have hardcoded lists around, even if that piece of code definitely covers the RTL locales we have (ar, fa, he, ur).

Trying to switch this question to Pike, in case he's aware of a way to do get direction from locale better than
http://searchfox.org/mozilla-central/source/browser/base/content/utilityOverlay.js#637-646
Flags: needinfo?(francesco.lodolo) → needinfo?(l10n)

Comment 5

9 months ago
You can check intl.uidirection.* prefs, which is what the chrome registry does.

Note, there are languages that exist in both LTR and RTL scripts (Kurdish, for one), so the mapping from language to direction isn't trivial.

It'd be nice if cld2 actually exposed which scripts it found, 'cause that's what it uses to hint at languages, but I don't see that ad-hoc.

You could probably mock that and just pick a few code points.

If this wasn't reader mode (and thus android, too), we could also think about using ICU, I guess. uscript_isRightToLeft exists as an API, at least.
Flags: needinfo?(l10n)
(In reply to Axel Hecht [:Pike] from comment #5)
> You can check intl.uidirection.* prefs, which is what the chrome registry
> does.

Not once bug 1312049 (re-)lands; those prefs are going away.
(Assignee)

Comment 7

9 months ago
According to Comment 5 and Comment 6, we have two options here.

1. We need to export the `uloc_isRightToLeft`[1](it is used in the patch of Bug 1312049) method for checking RTL.
2. We use the hardcode solution at this stage, kind of like we did in [2].

Since we might not know when the exporting solution could be done and resolved, I would say we could implement the hardcode solution first.

At the same time, we should file a new bug for exporting `isRightToLeft` method or other more suitable method. Once the bug is fixed, then we remove the hardcode solution.

What do you think?

[1]: http://searchfox.org/mozilla-central/source/intl/icu/source/common/unicode/uloc.h#897-912
[2]: http://searchfox.org/mozilla-central/source/browser/base/content/utilityOverlay.js#637-646
Flags: needinfo?(gijskruitbosch+bugs)

Comment 8

9 months ago
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #7)
> According to Comment 5 and Comment 6, we have two options here.
> 
> 1. We need to export the `uloc_isRightToLeft`[1](it is used in the patch of
> Bug 1312049) method for checking RTL.
> 2. We use the hardcode solution at this stage, kind of like we did in [2].
> 
> Since we might not know when the exporting solution could be done and
> resolved, I would say we could implement the hardcode solution first.
> 
> At the same time, we should file a new bug for exporting `isRightToLeft`
> method or other more suitable method. Once the bug is fixed, then we remove
> the hardcode solution.
> 
> What do you think?
> 
> [1]:
> http://searchfox.org/mozilla-central/source/intl/icu/source/common/unicode/
> uloc.h#897-912
> [2]:
> http://searchfox.org/mozilla-central/source/browser/base/content/
> utilityOverlay.js#637-646

This plan sounds sensible to me.
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

9 months ago
Hi Gijs,

I wrote a patch for the idea(Comment 3). Could you help review it? Thanks.
(Assignee)

Updated

9 months ago
See Also: → bug 1320265
(Assignee)

Comment 12

9 months ago
I've filed the exporting bug, Bug 1320265.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

9 months ago
mozreview-review
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review95676

::: toolkit/components/reader/AboutReader.jsm:128
(Diff revision 4)
> +    if (win.document.body.classList.contains("loaded")) {
> +      detect();
> +    } else {
> +      win.document.addEventListener("AboutReaderContentReady", detect);
> +    }

This doesn't make sense. We know we're always going to run before `_loadArticle`, so the body's classList will never contain "loaded". It also doesn't really make sense that we're listening for our own event here.

It looks like because we're constructing the narrate controls here, we need the promise to be ready already. But what I think would be clearer is to do something like this:

```js
this._languagePromise = new Promise(resolve => {
  this._foundLanguage = resolve;
});
```

here, and then in `_showContent`, call a new method called something like `_findLanguage`, that runs the language detection code after we've appended the main article text, calls `_foundLanguage()` with the language or null as you're doing here, and maybe saves it somewhere convenient for the directionality code to use.
Attachment #8814313 - Flags: review?(gijskruitbosch+bugs) → review-
(Assignee)

Comment 16

9 months ago
mozreview-review-reply
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review95676

> This doesn't make sense. We know we're always going to run before `_loadArticle`, so the body's classList will never contain "loaded". It also doesn't really make sense that we're listening for our own event here.
> 
> It looks like because we're constructing the narrate controls here, we need the promise to be ready already. But what I think would be clearer is to do something like this:
> 
> ```js
> this._languagePromise = new Promise(resolve => {
>   this._foundLanguage = resolve;
> });
> ```
> 
> here, and then in `_showContent`, call a new method called something like `_findLanguage`, that runs the language detection code after we've appended the main article text, calls `_foundLanguage()` with the language or null as you're doing here, and maybe saves it somewhere convenient for the directionality code to use.

There are two things here.

1. You're right. It(`win.document.body.classList`) will never cintain "loaded". So let's remove it.
2. In the patch, I just follow current code base logic to do language detection at same timing(before `this._loadArticle()`) as current code base (do language detection in `Narrator`[1]). So my question is why cannot we just follow same logic like we do now in code base and listen `AboutReaderContentReady` there? If we can follow it, I would say the solution in the patch is quite simple and OK. Does it make sense?

[1]: http://searchfox.org/mozilla-central/source/toolkit/components/narrate/Narrator.jsm#34-49

Comment 17

9 months ago
mozreview-review-reply
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review95676

> There are two things here.
> 
> 1. You're right. It(`win.document.body.classList`) will never cintain "loaded". So let's remove it.
> 2. In the patch, I just follow current code base logic to do language detection at same timing(before `this._loadArticle()`) as current code base (do language detection in `Narrator`[1]). So my question is why cannot we just follow same logic like we do now in code base and listen `AboutReaderContentReady` there? If we can follow it, I would say the solution in the patch is quite simple and OK. Does it make sense?
> 
> [1]: http://searchfox.org/mozilla-central/source/toolkit/components/narrate/Narrator.jsm#34-49

Because we don't actually do the detection here, we listen for an event here, and when the event fires, we do the language detection. This made sense when the code lived in the narrate module, because the narrate module had no other way of knowing when reader mode inserted the article content.

However, now we are moving the language detection code into reader mode code directly. That code is itself responsible for inserting the HTML that we then do language detection on. It would be needlessly complex for the code here to rely on an event that it fires itself, when it could just do the right thing immediately after having inserted the article text. The patch would be smaller and simpler if we could determine the language immediately - also because it avoids having to use the promise when determining the article direction (which is async, and might lead to a flash of wrongly-directed content).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

9 months ago
Updated patch for review comments.
(Assignee)

Comment 21

9 months ago
mozreview-review-reply
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review95676

> Because we don't actually do the detection here, we listen for an event here, and when the event fires, we do the language detection. This made sense when the code lived in the narrate module, because the narrate module had no other way of knowing when reader mode inserted the article content.
> 
> However, now we are moving the language detection code into reader mode code directly. That code is itself responsible for inserting the HTML that we then do language detection on. It would be needlessly complex for the code here to rely on an event that it fires itself, when it could just do the right thing immediately after having inserted the article text. The patch would be smaller and simpler if we could determine the language immediately - also because it avoids having to use the promise when determining the article direction (which is async, and might lead to a flash of wrongly-directed content).

Got it. Make sense, let's not depend on `AboutReaderContentReady` event. But we seems cannot determin the language without using a `promise`. The `LanguageDetector.detectLanguage` method just returns a `promise`.

How about we add a `callback` param into `_loadArticle` method, then we could do something like below to initialize the `NarrateControls` object?
```
this._loadArticle((article, languagePromise) => {
  if (win.speechSynthesis && Services.prefs.getBoolPref("narrate.enabled")) {
    new NarrateControls(mm, win, languagePromise);
  }
});
```

I've updated patch for the idea. Could you help review it? Thanks.

Comment 22

9 months ago
mozreview-review
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review95916

::: toolkit/components/reader/AboutReader.jsm:642
(Diff revision 6)
>        this._showError();
>        return;
>      }
>  
> -    this._showContent(article);
> +    var languagePromise = new Promise(resolve => {
> +      LanguageDetector.detectLanguage(article.content).then(result => {

article.content is the un-security-checked HTML version of the content. We should be passing the language promise the text content, not the HTML.

I suggested in comment 15 how to deal with the promise. Create a promise, and store the `resolve` method on `this`, then call it after determining the language. If the language determination is async itself, you can simply add another `.then` handler to its promise that resolves the promise we passed to the narrate code. This avoids having to pass along a callback through several function signatures.

::: toolkit/components/reader/AboutReader.jsm:732
(Diff revision 6)
> +        switch (language) {
> +          case "ar":
> +          case "fa":
> +          case "he":
> +          case "ug":
> +          case "ur":
> +            this._contentElement.setAttribute("dir", "rtl");
> +            this._headerElement.setAttribute("dir", "rtl");

```js
if (["ar", "fa", "he", "ug", "ur"].includes(language)) {
  ...
}
```

Have you checked whether the language detection code returns only a language code for these languages and not things like `ar-IR` or whatever? We might need to post-process for this matching.
Attachment #8814313 - Flags: review?(gijskruitbosch+bugs) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 24

9 months ago
mozreview-review-reply
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review95916

> article.content is the un-security-checked HTML version of the content. We should be passing the language promise the text content, not the HTML.
> 
> I suggested in comment 15 how to deal with the promise. Create a promise, and store the `resolve` method on `this`, then call it after determining the language. If the language determination is async itself, you can simply add another `.then` handler to its promise that resolves the promise we passed to the narrate code. This avoids having to pass along a callback through several function signatures.

Sure, got the idea. Misunderstood it previously. Already updated the patch for it.

> ```js
> if (["ar", "fa", "he", "ug", "ur"].includes(language)) {
>   ...
> }
> ```
> 
> Have you checked whether the language detection code returns only a language code for these languages and not things like `ar-IR` or whatever? We might need to post-process for this matching.

We imported Compact Language Library to do language detection on Bug 971047. I checked the source code we imported at that time, `enerated_language.cc`[1] lists all language codes it can detect. It doesn't have something like `ar-IR`, it contains `ar`, `fa`, `he`, `ug`, and `ur`. So I think the code is good here.

[1]: http://searchfox.org/mozilla-central/source/browser/components/translation/cld2/internal/generated_language.cc#648-1261
(Assignee)

Comment 25

9 months ago
Updated patch for review comments.
Comment hidden (mozreview-request)

Comment 27

9 months ago
mozreview-review
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review96434

::: toolkit/components/reader/AboutReader.jsm:726
(Diff revision 8)
> -    // Set "dir" attribute on content
> +      // Set "dir" attribute on content
> -    this._contentElement.setAttribute("dir", article.dir);
> +      this._contentElement.setAttribute("dir", article.dir);
> -    this._headerElement.setAttribute("dir", article.dir);
> +      this._headerElement.setAttribute("dir", article.dir);
> +    } else {
> +      this._languagePromise.then(language => {
> +        // TODO: Remove the below hard-code language codes once Bug 1320265 is resolved.

Nit: this comment would be slightly more idiomatic English if written like this:

// TODO: Remove the hardcoded language codes below once bug 1320265 is resolved.
Attachment #8814313 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 29

9 months ago
mozreview-review-reply
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review96434

> Nit: this comment would be slightly more idiomatic English if written like this:
> 
> // TODO: Remove the hardcoded language codes below once bug 1320265 is resolved.

Sure, updated patch for it. Thanks.
(Assignee)

Comment 30

9 months ago
I've fixed the nit and pushed a new try for it. If everything is good, please help land the patch. Thanks.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 31

9 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cbde1c9e78d5
Do language detection to get text direction, r=Gijs

Updated

9 months ago
Flags: needinfo?(gijskruitbosch+bugs)
Backed out for failing test_reader_view.html and test_session_scroll_position.html on Android:

https://hg.mozilla.org/integration/autoland/rev/52a5ff34b3b36e789ed21c2665889b8833ec61b0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cbde1c9e78d55907934892db41c6d2d5ac32a5e5
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7416448&repo=autoland

quote Gijs "we need to not use LanguageDetector on Android (it's desktop-only)"
Flags: needinfo?(evan)
(Assignee)

Comment 33

9 months ago
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #32)
> Backed out for failing test_reader_view.html and
> test_session_scroll_position.html on Android:
> 
> https://hg.mozilla.org/integration/autoland/rev/
> 52a5ff34b3b36e789ed21c2665889b8833ec61b0
> 
> Push with failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=cbde1c9e78d55907934892db41c6d2d5ac32a5e5
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=7416448&repo=autoland
> 
> quote Gijs "we need to not use LanguageDetector on Android (it's
> desktop-only)"

Hi Gijs,

Do we want to make `LanguageDetector` run on Android here?(I even don't know why LanguageDetector cannot run on Android?)

If so, let's file a new bug to do it. Or we just need to figure out a new way to get language code of an article?
Flags: needinfo?(evan) → needinfo?(gijskruitbosch+bugs)

Comment 34

9 months ago
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #33)
> (In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on
> intermittent or backout) from comment #32)
> > Backed out for failing test_reader_view.html and
> > test_session_scroll_position.html on Android:
> > 
> > https://hg.mozilla.org/integration/autoland/rev/
> > 52a5ff34b3b36e789ed21c2665889b8833ec61b0
> > 
> > Push with failures:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=autoland&revision=cbde1c9e78d55907934892db41c6d2d5ac32a5e5
> > Failure log:
> > https://treeherder.mozilla.org/logviewer.html#?job_id=7416448&repo=autoland
> > 
> > quote Gijs "we need to not use LanguageDetector on Android (it's
> > desktop-only)"
> 
> Hi Gijs,
> 
> Do we want to make `LanguageDetector` run on Android here?(I even don't know
> why LanguageDetector cannot run on Android?)

I don't know. It'll impact apk size. It's a decision for the android team, I guess. We can file a separate bug. For now, we should make sure that the code here doesn't use LanguageDetector if on android.
Flags: needinfo?(gijskruitbosch+bugs)

Updated

9 months ago
Blocks: 1265304
Summary: Do language detection for cases don't obtain a dir attribute → Do language detection for cases where we don't obtain a dir attribute
Comment hidden (mozreview-request)
(Assignee)

Comment 36

9 months ago
Got it. I've updated patch to do platform detection to ensure we only do language detection on Firefox desktop.

Please help review the code. If everything is OK, please help land it. Thanks.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Updated

9 months ago
See Also: → bug 1321477
(Assignee)

Comment 37

9 months ago
File a new bug(Bug 1321477) to determine language code of a webpage on Fennec.

Comment 38

9 months ago
mozreview-review
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review97074

::: toolkit/components/reader/AboutReader.jsm:20
(Diff revision 10)
>  XPCOMUtils.defineLazyModuleGetter(this, "AsyncPrefs", "resource://gre/modules/AsyncPrefs.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "NarrateControls", "resource://gre/modules/narrate/NarrateControls.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Rect", "resource://gre/modules/Geometry.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Task", "resource://gre/modules/Task.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "UITelemetry", "resource://gre/modules/UITelemetry.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "XulApp", "resource://gre/modules/commonjs/sdk/system/xul-app.jsm");

Where did this code come from? We shouldn't be using add-on SDK modules for this in Firefox/Toolkit code.

Instead, reuse `gIsFirefoxDesktop`, defined a few lines below.

::: toolkit/components/reader/AboutReader.jsm:821
(Diff revision 10)
>      this._doc.dispatchEvent(
>        new this._win.CustomEvent("AboutReaderContentReady", { bubbles: true, cancelable: false }));
>    },
>  
> +  _findLanguage: function(textContent) {
> +    XulApp.is("Firefox") && LanguageDetector.detectLanguage(textContent).then(result => {

Nit:

```js
if (gIsFirefoxDesktop) {
  LanguageDetector...
}
Attachment #8814313 - Flags: review+

Comment 39

9 months ago
mozreview-review-reply
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review97074

> Where did this code come from? We shouldn't be using add-on SDK modules for this in Firefox/Toolkit code.
> 
> Instead, reuse `gIsFirefoxDesktop`, defined a few lines below.

To be clear, you'll need to pull that constant out into global scope.

Updated

9 months ago
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
(Assignee)

Comment 41

9 months ago
mozreview-review-reply
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review97074

> To be clear, you'll need to pull that constant out into global scope.

Sure, I've updated for that.

> Nit:
> 
> ```js
> if (gIsFirefoxDesktop) {
>   LanguageDetector...
> }

Sure.
(Assignee)

Comment 42

9 months ago
I've updated patch for review comments. Please help review it again. Thanks.

Comment 43

9 months ago
mozreview-review
Comment on attachment 8814313 [details]
Bug 1318605 - Do language detection to get text direction,

https://reviewboard.mozilla.org/r/95580/#review97144

Nice, thanks!
Attachment #8814313 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 44

9 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/64cbe9e0ac0a
Do language detection to get text direction, r=Gijs

Comment 45

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/64cbe9e0ac0a
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 46

6 months ago
Verified on Aurora 53.0a2, with several devices, the estimated reading time follows the text direction, based on text language.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Blocks: 1347460
You need to log in before you can comment on or make changes to this bug.