setting vertical writing-mode on the document element doesn't result in expected horizontal scrollbar

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Depends on 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Assignee

Description

5 years ago
Testcase: load http://www.unicode.org/udhr/d/udhr_jpn.html, then use the Inspector to set the writing-mode of the <html> element to vertical-lr or vertical-rl.

Comment 2

5 years ago
This test case set writing-mode on a div (not on the html element). In my Nightly 37.0a1 (2014-12-09) I still get no horizontal scrollbar (and so the text gets cut off at the end of the viewport and I can't read the rest).

Updated

5 years ago
Attachment #8533508 - Attachment description: index.html → test case with writing-mode set on a div
Note that in attachment 8533492 [details] the vertical scroll-bar disappears when setting vertical writing-mode by pressing x, but on resizing the height of the viewport it reappears.

The testcase also triggers ASSERTION: Shouldn't be incomplete if availableBSize is UNCONSTRAINED.: 'aReflowState.AvailableBSize() != NS_UNCONSTRAINEDSIZE', file .../layout/generic/nsBlockFrame.cpp, line 1549, which may or may not be related.
Assignee

Comment 4

5 years ago
This fixes the content sizing and scrolling problems for vertical writing mode set on the root <html> element (rather than the <body>, not yet handled).
Attachment #8535493 - Flags: review?(smontagu)
Assignee

Updated

5 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee

Comment 5

5 years ago
This is a version of attachment 8533492 [details] where the writing mode is set directly on the <html> element rather than its <body> child. (I also added vertical-lr mode to the cycle, just for testing purposes -- I realise this isn't likely to be wanted for real Japanese content.)
Assignee

Comment 6

5 years ago
Note that bug 1102175 is also required for the vertical-rl case to work properly.
Depends on: 1102175
Assignee

Comment 7

5 years ago
Updated patch to handle the case where these frames have no contents to get the writing-mode from.
Attachment #8535530 - Flags: review?(smontagu)
Assignee

Updated

5 years ago
Attachment #8535493 - Attachment is obsolete: true
Attachment #8535493 - Flags: review?(smontagu)
Comment on attachment 8535530 [details] [diff] [review]
Make nsIFrame::GetWritingMode virtual, and override it in nsViewportFrame, nsCanvasFrame and nsHTMLScrollFrame to make them adopt the writing mode of their contents

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

LGTM, as far as it goes, but I agree with bug 1102175 comment 13 that we want to propagate from <body>
Attachment #8535530 - Flags: review?(smontagu) → review+
Assignee

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcc6081af275
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla37
https://hg.mozilla.org/mozilla-central/rev/bcc6081af275
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Created attachment 8535530 [details] [diff] [review]
> Make nsIFrame::GetWritingMode virtual, and override it in nsViewportFrame,
> nsCanvasFrame and nsHTMLScrollFrame to make them adopt the writing mode of
> their contents

Jonathan, do you recall why you made the nsCanvasFrame impl here jump from the frame tree --> content tree --> frame tree?  (as opposed to the ViewportFrame impl which just grabs the writing mode off of the first child)

Why couldn't nsCanvasFrame do the same thing that ViewportFrame does?

(Context: neerja's looking at GetWritingMode impls on print-related page frames on bug 1166147 -- and for some reason during printing, nsCanvasFrame::GetContent returns null.  That might just be a bug and we should make it not return null -- but I'm also curious whether we really need to depend on nsCanvasFrame::GetContent here.)
Flags: needinfo?(jfkthame)
(For reference, here's the frametree->contenttree->frametree nsCanvasFrame impl that I mentioned above:
 https://hg.mozilla.org/mozilla-central/rev/bcc6081af275#l1.12
vs. the frametree-only ViewportFrame impl:
 https://hg.mozilla.org/mozilla-central/rev/bcc6081af275#l5.12

In practice, it seems like the nsCanvasFrame's first child is always the root element's primary frame, so it seems like the ViewportFrame "defer to my first child" strategy would work (and be more direct) on nsCanvasFrame.  But I might be missing some subtlety.
Comment hidden (obsolete)
Assignee

Comment 14

3 years ago
Sorry, I don't recall any specifics. Most likely there's no strong reason, more like that was the implementation that arose most directly from figuring out what we needed to do here; but going directly to the first child may be a handy optimization.

(I don't know for sure whether "the nsCanvasFrame's first child is always the root element's primary frame", though it sounds likely to be true. Maybe it'd be worth reading some code and/or adding an assertion and running tests to try and confirm this.)
Flags: needinfo?(jfkthame)
Thanks, jfkthame.

FYI, Neerja's going to change this nsCanvasFrame implementation to more directly reference the root element (as PresContext()->Document()->GetRootElement()), in bug 1166147, since that's a more robust way to get at the root element (and its writing-mode).
It would be nice to have a reftest for this.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.