1127778 - Switching to reader mode changes paragraph display order

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect, P3)

Description

[Mark Capella [:capella]]

(Minor annoyance finally documented).

Using a simple 3-paragraph test [1], you can observe the order of paragraphs changing before [2] and after [3] entering readerMode.


[1] https://www.dropbox.com/s/0n0bec8x4p7i1je/readerReorder.html?dl=0
[2] https://www.dropbox.com/s/9msitzhmsw12o24/readerReorderBefore.png?dl=0
[3] https://www.dropbox.com/s/4q8mbuvgut3o79s/readerReorderAfter.png?dl=0

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Is this bug 809724?

Comment 2

[Mark Capella [:capella]]

Sounds like it.

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Using different tags. I don't think duping is the right process here.

Comment 4

[Brian Nicholson (:bnicholson)]

Minimal test case here: https://people.mozilla.org/~bnicholson/test/reader.html

This isn't a dupe of bug 809724 if that bug is due to mismatched tags (https://bugzilla.mozilla.org/show_bug.cgi?id=809724#c6). The test case linked above passes HTML validation.

Comment 5

[Brian Nicholson (:bnicholson)]

Interestingly, simply adding a byline fixes the order in this case: https://people.mozilla.org/~bnicholson/test/reader2.html

Comment 6

[:Gijs (he/him)]

This is because of the paging divs which somehow end up gathering up the other paras but not the second one, which then breaks stuff. In fact, I suspect I know what is the issue here (hint: removing things from an array you're looping over without being careful causes issues). Taking.

Comment 7

[:Gijs (he/him)]

Attachment: fix paragraph reordering issues caused by paging,

Comment 8

[:Gijs (he/him)]

Attachment: add more logging to readermode code so it's easier to debug issues,

Because logging is turned off anyway, I'd like to just land this change, too, so we don't have to add our own impromptu logging all the time when we investigate quality issues this week.

Comment 9

[:Gijs (he/him)]

(because there is also a github repo, I don't really know if this needs to go here or on github, but considering I'm working with the desktop codebase, I've assumed 'here' for now, please let me know ASAP if that assumption is broken)

Comment 10

[:Gijs (he/him)]

Attachment: PR

Comment 11

[:Margaret Leibovic]

Comment on attachment 8578755 [details] [review]
PR

I already merged this on github. r+