Closed Bug 1154295 Opened 6 years ago Closed 2 years ago

Reader Mode should not leave text/plain things in unstyled <pre> tags, so that the font isn't horrible

Categories

(Toolkit :: Reader Mode, defect, P3)

Product:
Toolkit
Component:
Reader Mode
Platform:
x86_64
Windows 8.1
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox68 --- verified

People

(Reporter: jaws, Assigned: trushita, Mentored)

References

Details

(Whiteboard: [about-reader-ui])

Attachments

(3 files, 1 obsolete file)

Bug 1154295:Added two helper functions to convert plain text document to HTML in Reader mode
47 bytes, text/x-phabricator-request
Details | Review
Bug #:1154295 Made changes in the helper functions that styles plain text document in reader mode
47 bytes, text/x-phabricator-request
Details | Review
Bug 1154295 - use normal formatting for plaintext documents in reader mode
47 bytes, text/x-phabricator-request
Details | Review 
STR:
Open http://www.gutenberg.org/cache/epub/48685/pg48685.txt
Look for Reader Mode icon in the location bar

ER:
The page should be readable

AR:
The page is not readable
Text files don't have <p> elements, it is simply one <pre /> with all of the text inside of it.
Duplicate of this bug: 1242302
Summary: Reader Mode should work for .txt files → Reader Mode should work for .txt/plain-text files
It now works, but it looks terrible.
Priority: -- → P3
Summary: Reader Mode should work for .txt/plain-text files → Reader Mode should not leave text/plain things in unstyled <pre> tags, so that the font isn't horrible
Whiteboard: [about-reader-ui]
Duplicate of this bug: 1301338
Mentor: gijskruitbosch+bugs

Hi, can I work on this issue?

(In reply to trushita from comment #8)

Hi, can I work on this issue?

Sure. This is quite a tricky bug to fix. We'll need to detect that we're being fed a text/plain document (can probably do that using document.contentType that's being treated as XHTML, that has a <body> with a single <pre> tag, which has no element children).

Then we'll need to transform that somehow. I expect that what'd more or less work is if we split the contents by any double newlines (/\r?\n\r?\n/) and made each resulting block of text a paragraph (<p>). For each paragraph, we'd probably want to make sure that any remaining newlines in them would be replaced by a <br>.

We'd probably want to make such changes in https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/toolkit/components/reader/ReaderMode.jsm#363 , coming up with a isDocumentPlainText() helper that checks for the right document, and if we get that document, return early, returning the result of another helper, convertPlainTextDocument which we'd have to write and would do what I suggested in the previous paragraph.

You should be able to use artifact builds for this. Does that sound doable? Do you need more help?

Flags: needinfo?(trushita)

(In reply to :Gijs (he/him) from comment #9)

(In reply to trushita from comment #8)

Hi, can I work on this issue?

Sure. This is quite a tricky bug to fix. We'll need to detect that we're being fed a text/plain document (can probably do that using document.contentType that's being treated as XHTML, that has a <body> with a single <pre> tag, which has no element children).

Then we'll need to transform that somehow. I expect that what'd more or less work is if we split the contents by any double newlines (/\r?\n\r?\n/) and made each resulting block of text a paragraph (<p>). For each paragraph, we'd probably want to make sure that any remaining newlines in them would be replaced by a <br>.

We'd probably want to make such changes in https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/toolkit/components/reader/ReaderMode.jsm#363 , coming up with a isDocumentPlainText() helper that checks for the right document, and if we get that document, return early, returning the result of another helper, convertPlainTextDocument which we'd have to write and would do what I suggested in the previous paragraph.

You should be able to use artifact builds for this. Does that sound doable? Do you need more help?

Thanks. I'll look into it. I'll let you know if I have any doubts.

Flags: needinfo?(trushita)

I went through the link
http://www.gutenberg.org/cache/epub/48685/pg48685.txt but I can see it is readable. Does that mean we need to make it more styled?

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to trushita from comment #11)

I went through the link
http://www.gutenberg.org/cache/epub/48685/pg48685.txt but I can see it is readable. Does that mean we need to make it more styled?

Yes, we need to make it look more like what e.g. https://www.gijsk.com/blog/2019/02/getting-firefox-artifact-builds-working-on-an-arm64-aarch64-windows-device/ looks like when you put it in reader mode. The fonts should be nice, and adjust when using the sidebar to change them, etc. None of that is working for plain text files like the gutenberg stuff today.

Flags: needinfo?(gijskruitbosch+bugs)

Yes, I am working on it.

Will the text document thats being treated as XHTML have a single <pre> tag only in body?

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to trushita from comment #16)

Will the text document thats being treated as XHTML have a single <pre> tag only in body?

Yes.

(In reply to Monika Maheshwari [:MonikaMaheshwari] from comment #17)

Hey I have finished working on that bug. Can you assign me something else.

This isn't really on-topic for this bug; I'll respond on your other bug.

Flags: needinfo?(gijskruitbosch+bugs)

I am done with the helper functions -isDocumentPlainText() and convertPlainTextDocument(). I am currently trying to understand to integrate in the existing file. I'll let you know if I get stuck.

(In reply to trushita from comment #19)

I am done with the helper functions -isDocumentPlainText() and convertPlainTextDocument(). I am currently trying to understand to integrate in the existing file. I'll let you know if I get stuck.

FWIW, I think we basically want to check isDocumentPlainText and then call convertPlainTextDocument at:

https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/toolkit/components/reader/ReaderMode.jsm#384

ie before doing the XML serialization step. Hope that helps.

Before this change the plain text document in reader mode was not formatted/styled properly. Modified the existing code to style the document in reader mode.

My bad, I need to make a few changes to the path

(In reply to trushita from comment #22)

My bad, I need to make a few changes to the path.

I mean patch

Assignee: nobody → trushita

I am a little confused about using pElem.hasChildNodes(). Once we split the paragraphs then we are splitting each paragraph on new single lines using split(). So now the inner loop will be like :-

for (let line of lines) {
		pElem.append(line);
		let brElem = document.createElement('br');
		pElem.append(brElem);
	}

Here, we are appending the br element after each line in the inner loop since it splits on new line.

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to trushita from comment #24)

I am a little confused about using pElem.hasChildNodes(). Once we split the paragraphs then we are splitting each paragraph on new single lines using split(). So now the inner loop will be like :-

for (let line of lines) {
		pElem.append(line);
		let brElem = document.createElement('br');
		pElem.append(brElem);
	}

Here, we are appending the br element after each line in the inner loop since it splits on new line.

Ah, yes, this is simpler and better. I thought having a trailing <br> would add extra space, but it seems it doesn't. Thanks!

Flags: needinfo?(gijskruitbosch+bugs)

Fixes done to the helper functions that styles plain text document in reader mode. Before this change the plain text document in reader mode was not formatted/styled properly.

Before this change the plain text document in reader mode was not formatted/styled properly. Modified the existing code to style the document in reader mode. Added two helper functions to convert plain text document to XHTML document. Later, refactored helper function code.

Attachment #9052946 - Attachment description: Bug 1154295: Made changes in the helper functions that styles plain text document in reader mode → Bug 1154295 - use normal formatting for plaintext documents in reader mode
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0539b9847dd5
use normal formatting for plaintext documents in reader mode r=Gijs

Thank you for mentoring me. The work was interesting,if there is any other bug I can work on, please let me know.

Flags: needinfo?(gijskruitbosch+bugs)

https://hg.mozilla.org/mozilla-central/rev/0539b9847dd5

Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

(In reply to trushita from comment #29)

Thank you for mentoring me. The work was interesting,if there is any other bug I can work on, please let me know.

I see you pinged Jared on bug 1485893 - that should work, I think. Let me know if not.

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to :Gijs (he/him) from comment #31)

(In reply to trushita from comment #29)

Thank you for mentoring me. The work was interesting,if there is any other bug I can work on, please let me know.

I see you pinged Jared on bug 1485893 - that should work, I think. Let me know if not.

That bug seems to be fixed already. I just got a green sign today for bug 1502134 :-https://bugzilla.mozilla.org/show_bug.cgi?id=1502134 so i'll just check that out. If I do need another bug, I hope it would be okay to ping you. Also, Is there any other place I can ping you considering I am commenting out of topic here. Thanks for the help.

(In reply to trushita from comment #32)

(In reply to :Gijs (he/him) from comment #31)

(In reply to trushita from comment #29)

Thank you for mentoring me. The work was interesting,if there is any other bug I can work on, please let me know.

I see you pinged Jared on bug 1485893 - that should work, I think. Let me know if not.

That bug seems to be fixed already. I just got a green sign today for bug 1502134 :-https://bugzilla.mozilla.org/show_bug.cgi?id=1502134 so i'll just check that out. If I do need another bug, I hope it would be okay to ping you. Also, Is there any other place I can ping you considering I am commenting out of topic here. Thanks for the help.

IRC is probably best, otherwise email - gijs @ mozilla (.com) works. Or my bugmail address without the "+bugs".

Flags: qe-verify+

I have reproduced this issue using Firefox 66.0a1 (2019.01.02) on Win 8.1 x64 and Win 10 x64.
I can confirm this issue is fixed, I verified using Firefox 68.0b7 on Win 10 x64, Win 8.1 x64, macOS 10.13.6 and Ubuntu 18.04 x64. Now the text file have <p> elements, the page is readable in Reader Mode, the fonts were changed, but toggled for second time the Reader View button this message appear "Failed to load article from page" , reproduced bug 1331981.

Status: RESOLVED → VERIFIED
status-firefox68: fixed → verified
Flags: qe-verify+
Regressions: 1645607
You need to log in before you can comment on or make changes to this bug.