Closed Bug 1108067 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Layout: Block and Inline, defect)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla37

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(4 files, 1 obsolete file)

test case demonstrating the same problem (perhaps)
37.68 KB, text/html
Details
test case with writing-mode set on a div
37.73 KB, text/html
Details
testcase for vertical writing mode set on the document element
37.80 KB, text/html
Details
Make nsIFrame::GetWritingMode virtual, and override it in nsViewportFrame, nsCanvasFrame and nsHTMLScrollFrame to make them adopt the writing mode of their contents
6.28 KB, patch
smontagu
: review+
Details | Diff | Splinter Review 
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.
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).
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.
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: nobody → jfkthame
Status: NEW → ASSIGNED
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.)
Note that bug 1102175 is also required for the vertical-rl case to work properly.
Depends on: 1102175
Updated patch to handle the case where these frames have no contents to get the writing-mode from.
Attachment #8535530 - Flags: review?(smontagu)
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+
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: 10 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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: