37.68 KB, text/html
37.73 KB, text/html
37.80 KB, text/html
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
|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).
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)
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+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla37
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.)
(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.)
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.
You need to log in before you can comment on or make changes to this bug.