1323331 - Improve reader mode code dealing with detected language/direction

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, task, P5)

Comment 1

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 2

[Harold Camacho]

Hi Gijs, I am a new contributor and would like to work on this bug. Could you please provide a bit of context for this one?

Thanks!

Comment 3

[:Gijs (back August 25; he/him)]

(In reply to Harold Camacho from comment #2)

Hi Gijs, I am a new contributor and would like to work on this bug. Could you please provide a bit of context for this one?

Thanks!

Hm, well, I think past me was a bit confused. I think the bug was filed as a result of my comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1265304#c35 . So this is about the _languagePromise set up here and resolved here. The promise is passed to NarrateControls here.

Past me thought that we could get rid of the promise and just pass the language, which we know from the article. But we only load the article after we construct the narratecontrols, and anyway loading the article is asynchronous - we have to fetch stuff from the parent process into the child or from the internet, in some cases (e.g. session restore of reader mode states, when the non-reader mode page is not first available, reader mode has to go ask for it).

That doesn't seem possible.

Still, it would be nice to refactor this to use Promise.withResolvers() instead of the manual double properties (_foundLanguage and _languagePromise. It would also be nice to rename article.language to article.detectedLanguage (we'll need to update the promise to resolve with that new property, as well as update the code that sets it in ReaderMode.sys.mjs) and change https://searchfox.org/mozilla-central/rev/d602f8558872d133dc9240a01cd25d0898c58e5a/toolkit/components/reader/ReaderMode.sys.mjs#470-478 to use Services.intl.getScriptDirection instead. Does that seem like something you could write a patch for?

Comment 4

[Harold Camacho]

Sure Gijs, I can write a patch for this one! Thanks for the detailed explanation!

Comment 5

[Harold Camacho]

Attachment: Bug 1323331 - Improve reader mode code dealing with detected language/direction. r?Gijs!

Comment 6

[Harold Camacho]

Hi Gijs, I've updated the files to reflect the suggested changes. Are there any specific tests you'd like me to run before I submit this for formal review?

Thanks!

Comment 7

[:Gijs (back August 25; he/him)]

(In reply to Harold Camacho from comment #6)

Hi Gijs, I've updated the files to reflect the suggested changes. Are there any specific tests you'd like me to run before I submit this for formal review?

Thanks!

Running ./mach test toolkit/components/reader/ should cover it, thanks!

Comment 8

[Harold Camacho]

Hey Gijs, I've made some updates to my patch to ensure all the tests pass. After running the full test suite, everything appears to be working as expected.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86d8129309e2
Improve reader mode code dealing with detected language/direction. r=Gijs

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef0534a6573d
Backed out changeset 86d8129309e2 for causing bc failures @browser_link_preview.js. CLOSED TREE

Comment 11

[agoloman]

Backed out for causing bc failures @browser_link_preview.js.

• Backout link
• Push with failures
• Failure Log

Comment 12

[Harold Camacho]

Hi Gijs, could you provide some guidance on addressing the above issue? Thanks!

Comment 13

[:Gijs (back August 25; he/him)]

(In reply to Harold Camacho from comment #12)

Hi Gijs, could you provide some guidance on addressing the above issue? Thanks!

Huh, that's an interesting failure. The first step would be understanding why that test fails, which is not so clear to me.

It turns out that https://searchfox.org/mozilla-central/rev/19764d620c02025bdcc8d1f3c4fcf5a580407a01/browser/components/genai/LinkPreviewChild.sys.mjs#280-293 uses reader mode for link previews. So that code also depends on article.language but it's not obvious because of the deconstructing assignment.

You can try to reproduce the issue locally (with ./mach test browser_link_preview.js) and then fix that code to use detectedLanguage as we renamed the property, using ./mach build faster to rebuild, and then re-running the test to check it's fixed with that change. Then if you submit an update to the patch you already have to phab, I can try relanding it.

Comment 14

[Harold Camacho]

Hi Gijs,

While trying to follow your advice, I noticed that my current checkout was missing browser_link_preview.js and LinkPreviewChild. I pulled and merged central into my feature branch. I then updated the LinkPreviewChild code by changing the reference from language to detectedLanguage as you suggested.

After applying those changes, I built and ran ./mach test browser_link_preview.js, which reproduced the same errors described in the backout log. So unfortunately, even with the updated property, the failures persisted.

I also ran ./mach test toolkit/components/reader/ to confirm that my original ReaderMode changes were still working. However, a number of tests failed this time. A recurring error across the suite was:

console.error: "Error in ReaderWorker: " (new Error("Failed to load resource://gre/modules/reader/ReaderWorker.sys.mjs", "moz-src:///toolkit/components/reader/ReaderMode.sys.mjs", 415))

Alongside that, every failing test also included:

JavaScript error: resource:///actors/AboutReaderParent.sys.mjs, line 86: TypeError: message.data is null

I haven’t made any changes beyond the merge and the detectedLanguage updates, so I’m not sure what could be causing these new failures. From the error messages, it looks like ReaderWorker may not be loading correctly within ReaderMode, but I haven’t found anything obvious yet.

Would appreciate any thoughts you might have on this.

Thanks!

Comment 15

[:Gijs (back August 25; he/him)]

Did you re-run ./mach build after pulling and merging central? If the file didn't load I wonder if the build is not quite up-to-date.

With regards to the patch, if the build isn't up-to-date that would probably help explain, but if you still have issues after fixing that it'd be good to update the patch on phabricator so I can take a look. :-)

Comment 16

[Harold Camacho]

I ran into some issues with Mercurial that caused the problem I mentioned earlier haha—fortunately, I was able to sort them out. I also noticed there was one more place where we needed to update article.language to use the new detectedLanguage key, and that was in LinkPreview. After making that change, all the tests passed as expected.
Thanks again for your guidance!

Comment 17

[Harold Camacho]

Hi Gijs, just a quick check-in on the patch for Bug 1323331 (D244820) — all tests are passing and everything looks good to go. Thanks!

Comment 18

[:Gijs (back August 25; he/him)]

(In reply to Harold Camacho from comment #17)

Hi Gijs, just a quick check-in on the patch for Bug 1323331 (D244820) — all tests are passing and everything looks good to go. Thanks!

Hey! Sorry for the delay, I was out from Wednesday last week for some medical stuff. Back now, and I prodded some people and just queued this to re-land. Thanks!

Comment 19

[Harold Camacho]

No worries at all, and thank you, Gijs! I really appreciate you following up and queuing the patch to re-land. Thanks again!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f22e82031bd5
Improve reader mode code dealing with detected language/direction. r=Gijs,firefox-ai-ml-reviewers,Mardak

Comment 21

[amarc]

https://hg.mozilla.org/mozilla-central/rev/f22e82031bd5