1331015 - Reader Mode does not respect vertical writing mode

Status: NEW

(Toolkit :: Reader Mode, defect, P3)

Description

[Jonathan Kew [:jfkthame]]

When viewing a page that uses vertical writing mode, such as http://www.mongolfont.com/mn/news/altansuh/, and then entering Reader View, the vertical writing mode is ignored and the content displayed horizontally.

In the case of vertically-written CJK pages, this will usually still result in readable content, although IMO if the original document was set to vertical mode at the document root (not just a sub-element within an otherwise-horizontal doc), Reader View should respect this.

In the case of Mongolian content such as the URL given, however, it may result in completely broken content, as Mongolian fonts may not be designed to support horizontal display at all.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: sample Japanese document in vertical writing mode

Here's an example of a Japanese document with vertical writing mode set on the root element; IMO, this should result in Reader View also using vertical mode to display the content, but currently it appears horizontally.

Comment 2

[Jonathan Kew [:jfkthame]]

Hmm, thinking about this a bit, I suspect it may be trickier than it seemed at first glance. When we parse a document for Reader View and extract what RV is going to present, we presumably ignore CSS styling altogether; but vertical writing mode is specified via styles, not in the HTML itself (unlike bidi direction, which is specified via an HTML attribute).

In the case where a document is already open in a tab, and the user clicks the Reader icon in the URL bar (or chooses the menu item), I suppose we could look at the writing-mode of the current presentation and pass that somehow to the extracted Reader content (ignoring the possibility that the doc might use media queries, for example, to present itself in different writing modes for different contexts); but if the user goes directly to an about:reader?url=... location, we may be loading a document for Reader View without having any styled presentation already on hand, so there's no writing-mode value available.

Comment 3

[:Gijs (he/him)]

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> Hmm, thinking about this a bit, I suspect it may be trickier than it seemed
> at first glance. When we parse a document for Reader View and extract what
> RV is going to present, we presumably ignore CSS styling altogether; but
> vertical writing mode is specified via styles, not in the HTML itself
> (unlike bidi direction, which is specified via an HTML attribute).
> 
> In the case where a document is already open in a tab, and the user clicks
> the Reader icon in the URL bar (or chooses the menu item), I suppose we
> could look at the writing-mode of the current presentation and pass that
> somehow to the extracted Reader content (ignoring the possibility that the
> doc might use media queries, for example, to present itself in different
> writing modes for different contexts); but if the user goes directly to an
> about:reader?url=... location, we may be loading a document for Reader View
> without having any styled presentation already on hand, so there's no
> writing-mode value available.

You're correct. We could in principle check the original document in the more common case, and make an educated guess based on language (which we already detect in order to do RTL right in similar circumstances**) in the other? Is there a reasonable (ie "usually correct") mapping from language to writing modes?


** direction which yes, ideally should be specified in an attribute, but isn't always... cf. https://bugzilla.mozilla.org/show_bug.cgi?id=1173548#c1

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to :Gijs from comment #3)
> (In reply to Jonathan Kew (:jfkthame) from comment #2)
> > Hmm, thinking about this a bit, I suspect it may be trickier than it seemed
> > at first glance. When we parse a document for Reader View and extract what
> > RV is going to present, we presumably ignore CSS styling altogether; but
> > vertical writing mode is specified via styles, not in the HTML itself
> > (unlike bidi direction, which is specified via an HTML attribute).
> > 
> > In the case where a document is already open in a tab, and the user clicks
> > the Reader icon in the URL bar (or chooses the menu item), I suppose we
> > could look at the writing-mode of the current presentation and pass that
> > somehow to the extracted Reader content (ignoring the possibility that the
> > doc might use media queries, for example, to present itself in different
> > writing modes for different contexts); but if the user goes directly to an
> > about:reader?url=... location, we may be loading a document for Reader View
> > without having any styled presentation already on hand, so there's no
> > writing-mode value available.
> 
> You're correct. We could in principle check the original document in the
> more common case,

That would be good, I think.

> and make an educated guess based on language (which we
> already detect in order to do RTL right in similar circumstances**) in the
> other? Is there a reasonable (ie "usually correct") mapping from language to
> writing modes?

This would be harder. Vertical mode is used for Chinese and Japanese, but so is horizontal mode, and I don't think it's at all clear that running Reader in vertical mode just based on language would be a good idea there. (If anything, consult with the JA and ZH communities to see what they think.)

The only case where vertical is clearly the right choice, AFAIK, would be for Mongolian _when written in traditional Mongolian script_ and related cases (e.g. Manchu). But here, language is not a sufficient indicator, as Mongolian is also (more?) commonly written in Cyrillic script, for which vertical would be wrong. So we'd need to look at the (dominant) script used in the content, not just the language. This sounds like it's getting considerably trickier (and more expensive) to analyze.

So I think the pragmatic thing to do is probably to only handle the case of the user choosing Reader mode for a page that is already open in a tab, in which case we look at the writing mode of its root element (or the "root" of the content that we extract as the Reader-mode article) and use that.

Comment 5

[Jonathan Kew [:jfkthame]]

Aside from the problem of determining the writing-mode to use, a secondary issue is that the CSS used for the Reader view has some horizontal assumptions built into it, such that if we do set 'writing-mode:vertical-rl' on the root <html> element, the resulting display is somewhat broken. Some minor conversions from physical to logical properties in the stylesheet will help greatly there.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Existing Reader View styling when vertical writing mode is applied

Here's what the sample Japanese document above currently looks like if we go into Reader View and then (via DevTools) set writing-mode:vertical-rl on the <html> element. Spacing etc. around the title is poor, and (most obviously) the Reader View controls are messed up.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - Use some logical (instead of physical) properties in Reader View styling, to make it work better in a vertical writing-mode context

Here are some minimal fixes to the stylesheet that make things much better in vertical mode. (They should have no impact on horizontal mode, as the logical properties will resolve to the same as the former physical styles.)

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Reader View in vertical mode with patched stylesheet

Here's a screenshot of the same document with writing-mode:vertical-rl applied to the root <html> element in the reader view, after updating the stylesheet to use logical properties (and to ensure the panel of controls keeps its expected horizontal mode).

Comment 9

[:Gijs (he/him)]

Comment on attachment 8828271 [details] [diff] [review]
pt 1 - Use some logical (instead of physical) properties in Reader View styling, to make it work better in a vertical writing-mode context

Review of attachment 8828271 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but you probably want to make a similar patch for https://dxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReaderControls.css .

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to :Gijs from comment #9)
> you probably want to make a similar patch for
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/
> aboutReaderControls.css .

Yes, I expect so, but I don't have an up-to-date android build to test with just now. I'll see if I can get a build going without too much pain...

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - Use logical properties in the Android version of aboutReaderControls.css

This seems to work as expected on Android (when setting vertical writing mode on the root <html> element using WebIDE, for testing purposes). Again, there should be no change to horizontal mode.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - Logicalize some more aboutReader styling, for improved vertical writing-mode support

Some further adjustments to improve the vertical-mode styling, this time in aboutReader.css, which also has a bunch of physical margin/width properties that should be converted to logical.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8828317 [details] [diff] [review]
pt 3 - Logicalize some more aboutReader styling, for improved vertical writing-mode support

Review of attachment 8828317 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/themes/core/aboutReader.css
@@ +72,5 @@
>  /* Override some controls and content styles based on color scheme */
>  
>  body.light > .container > .header > .domain {
>    color: #ee7600;
> +  border-block-end-color: #d0d0d0; 

Nit: here and below, can you remove the trailing whitespace?

Comment 14

[:Gijs (he/him)]

Comment on attachment 8828316 [details] [diff] [review]
pt 2 - Use logical properties in the Android version of aboutReaderControls.css

Review of attachment 8828316 [details] [diff] [review]:
-----------------------------------------------------------------

Going to be cheeky and r+ this, though I'm not technically an android peer. It's just CSS, what could go wrong, etc. :-)

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - Apply the article's writing-mode (if known) to the Reader View root element

So now, all it takes to get a vertical-mode Reader View is to set the appropriate writing-mode on the root. This works as expected if I manually hack the return in Readability.parse() to add writingMode: 'vertical-rl' to the object it returns; but at this point I don't know how to make parse() actually look for an existing styled presentation of the content and retrieve the writing-mode from there. So that's the remaining piece of the puzzle needed here.

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to :Gijs from comment #14)
> It's just CSS, what could go wrong, etc. :-)

Ummm... indeed! ;-)

OK, so what remains to be done here is to actually check the writing-mode of the content that we decide to present, if indeed the document is loaded in a tab and thus has styles available. Unfortunately, I don't really have any idea how to go about that... this toolkit code is largely a mystery to me. Are you up for tackling this, or can you point to a suitable victim? :)

Comment 17

[:Gijs (he/him)]

Comment on attachment 8828362 [details] [diff] [review]
pt 4 - Apply the article's writing-mode (if known) to the Reader View root element

Review of attachment 8828362 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me . I'll see if I can poke at the missing piece for a bit.

Comment 18

[:Gijs (he/him)]

In practice, where do people set writing-mode? Is it likely to be on the root or <body>, or do people mix writing modes often and just assign them to specific bits of content? I realize it's possible to mix them and so in principle we should cater for that, but checking a few nodes is easier than checking everything, and I don't know that there'd otherwise be a good way of making this determination, unfortunately... Ideally, what we'd want is an enumeration of nodes that are specifically styled with a writing-mode style, but it's not clear to me how to easily obtain that information... the frame tree isn't directly script-accessible. We could walk stylesheets and then do querySelectorAll for nodes that match that, keep those references around and then see if content we get back from readability is in there, but that seems... horrible? :-)

Comment 19

[:Gijs (he/him)]

Evan, do you have ideas that might help here?

Comment 20

[Jonathan Kew [:jfkthame]]

For a "globally vertical" site, I believe writing-mode is usually set on either the root or <body> element (recommended practice is to put it on the root, but there's a legacy pattern of setting it on <body>). At http://mongolfont.com/, for example, it is set using

  * { writing-mode: vertical-lr; }

which is rather overkill, but does mean that it'll be present everywhere from the root on down the tree. :)

Often, though, the site may have some non-vertical-mode structure around the content, such as http://kurage.hatenablog.com/entry/2013/03/24/134149. Here, the writing-mode is set on the <div class="text-content"> that provides the main text that Readability extracts; it's also set (separately) on the header <h1 class="entry-title"> that we detect and use as the title, though it is not set on the common ancestor of these elements. So I don't know if it's feasible for us to detect this case as a "vertical mode article", depending quite how the Readability parsing and extraction works, but I think it'd be highly desirable if we could do so.

I'm not sure we should even try to handle fully mixed-writing-mode cases (preserving the mixture in the Reader view) -- e.g. imagine a horizontal-mode article that happens to contain a number of <blockquote>s in Chinese, where those have been styled vertically. Or consider https://en.wikipedia.org/wiki/Manchu_language, where there are a few short fragments of Manchu in its native script, styled with vertical writing mode. I don't think Reader mode can realistically be expected to track this. At least not for a first effort... :)

Comment 21

[:Gijs (he/him)]

Ni to remind myself to come back to this.

Comment 22

[:Gijs (he/him)]

Thinking about this more, for text-direction we have pseudoselectors (:dir(rtl)). How hard would it be to add one for vertically written text? That would really help here because we'd be able to query the DOM for nodes with non-default writing-modes.

Comment 23

[Jonathan Kew [:jfkthame]]

Unfortunately, I think that's going to be hard.

The :dir pseudo is feasible because it matches directionality defined in the document via the dir attribute -- possibly inherited from a parent element, or computed from content in the case of dir=auto -- but not directionality specified by styling using the CSS 'direction' property. So matching :dir does not require style resolution, AFAIK, or indeed the presence of a stylesheet at all; it is based purely on the document's markup/content.

Writing-mode, OTOH, is not represented in any way in HTML; it is purely a style property. So a pseudo such as :writing-mode(vertical) would require style resolution before it could be matched at all.

Comment 24

[Jintao Hu]

Attachment: Bug 1331015 - Recognize and apply vertical writing mode in reader mode, and fix logical properties. r=niklas,mtigley

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:Gijs, could you please find another way to get the information or close the bug as INCOMPLETE if it is not actionable?

For more information, please visit auto_nag documentation.

Comment 26

[:Gijs (he/him)]

Niklas, can we rebase and land this stuff?

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P2'.
:Gijs, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 28

[:Gijs (he/him)]

This is already pending needinfo from Niklas, so clearing mine.

Comment 29

[BugBot [:suhaib / :marco/ :calixte]]

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