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

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) 10 years ago Michael[tm] Smith [:sideshowbarker] (deleted), text/html		Details
test case with writing-mode set on a div 10 years ago Michael[tm] Smith [:sideshowbarker] (deleted), 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 10 years ago Jonathan Kew [:jfkthame] (deleted), patch		Details \| Diff \| Splinter Review
testcase for vertical writing mode set on the document element 10 years ago Jonathan Kew [:jfkthame] (deleted), 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 10 years ago Jonathan Kew [:jfkthame] (deleted), patch	smontagu : review+	Details \| Diff \| Splinter Review

Jonathan Kew [:jfkthame]

Assignee

Description

•

10 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.

Michael[tm] Smith [:sideshowbarker]

Comment 1

•

10 years ago

Attached file test case demonstrating the same problem (perhaps) (deleted) — Details

Michael[tm] Smith [:sideshowbarker]

Comment 2

•

10 years ago

Attached file test case with writing-mode set on a div (deleted) — Details

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).

Michael[tm] Smith [:sideshowbarker]

Updated

•

10 years ago

Attachment #8533508 - Attachment description: index.html → test case with writing-mode set on a div

Simon Montagu :smontagu

Comment 3

•

10 years ago

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.

Jonathan Kew [:jfkthame]

Assignee

Comment 4

•

10 years ago

Attached patch Make nsIFrame::GetWritingMode virtual, and override it in nsViewportFrame, nsCanvasFrame and nsHTMLScrollFrame to make them adopt the writing mode of their contents (obsolete) (deleted) — Details — Splinter Review

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)

Jonathan Kew [:jfkthame]

Assignee

Updated

•

10 years ago

Assignee: nobody → jfkthame

Status: NEW → ASSIGNED

Jonathan Kew [:jfkthame]

Assignee

Comment 5

•

10 years ago

Attached file testcase for vertical writing mode set on the document element (deleted) — Details

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.)

Jonathan Kew [:jfkthame]

Assignee

Comment 6

•

10 years ago

Note that bug 1102175 is also required for the vertical-rl case to work properly.

Depends on: 1102175

Jonathan Kew [:jfkthame]

Assignee

Comment 7

•

10 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)

Jonathan Kew [:jfkthame]

Assignee

Updated

•

10 years ago

Attachment #8535493 - Attachment is obsolete: true

Attachment #8535493 - Flags: review?(smontagu)

Simon Montagu :smontagu

Comment 8

•

10 years ago

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+

Jonathan Kew [:jfkthame]

Assignee

Comment 9

•

10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcc6081af275

OS: Mac OS X → All

Hardware: x86 → All

Target Milestone: --- → mozilla37

Ryan VanderMeulen [:RyanVM]

Comment 10

•

10 years ago

https://hg.mozilla.org/mozilla-central/rev/bcc6081af275

Status: ASSIGNED → RESOLVED

Closed: 10 years ago

Resolution: --- → FIXED

Daniel Holbert [:dholbert]

Comment 11

•

8 years ago

(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)

Daniel Holbert [:dholbert]

Comment 12

•

8 years ago

(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)

Also for reference, I'd like to add that when using print preview (on Mac), the frame tree dump looks like: Viewport(-1)@124771b68 [view=11d3f4700] {0,0,48960,63360} wm=v-rl-ltr: logical size={63360,48960} [state=000b063000002623] [sc=124771720:-moz-viewport^0]< SimplePageSequence(html)(-1)@124771fe8 {0,0,0,0} wm=v-rl-ltr: logical size={0,0} parent wm=v-rl-ltr, cs={48960,63360}, logicalRect={0,48960,0,0} [state=000b020000000403] [content=1277f9690] [sc=124771140:-moz-page-sequence]< Page(-1)@124772788 {1440,720,0,0} wm=v-rl-ltr: logical size={0,0} parent wm=v-rl-ltr, cs={0,0}, logicalRect={720,-1440,0,0} [state=000b020000000403] [sc=124771f20:-moz-page]< PageContent(-1)@124772cd0 {3840,3848,41272,55664} wm=v-rl-ltr: logical size={55664,41272} parent wm=v-rl-ltr, cs={0,0}, logicalRect={3848,-45112,55664,41272} [state=000b002000000603] [sc=124772a00:-moz-pagecontent]< Canvas(-1)@124772e98 {0,0,0,0} parent wm=v-rl-ltr, cs={41272,55664}, logicalRect={0,41272,0,0} [state=000b002000000602] [sc=124772df0:-moz-canvas]< Block(html)(-1)@1248f90c8 {0,0,0,0} wm=v-rl-ltr: logical size={0,0} [state=000b160000d00602] [content=1277f9690] [sc=1248f9020^0]< line 124af1d30: count=1 state=block,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x9] {0,0,0,0} < Block(body)(2)@1248f9e10 {0,0,0,0} wm=v-rl-ltr: logical size={0,0} parent wm=v-rl-ltr, cs={0,0}, logicalRect={0,0,0,0} [state=000b120000100602] [content=1267fd700] [sc=1248f9aa8]< line 124af1ba0: count=1 state=block,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x9] {0,0,0,0} < Block(div)(0)@1248fa518 next=124a1c698 {0,0,0,0} wm=v-rl-ltr: logical size={0,0} parent wm=v-rl-ltr, cs={0,0}, logicalRect={0,0,0,0} [state=000b120000100602] [content=1247c5d70] [sc=1248f9f08]< line 124a1c558: count=1 state=block,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x9] {0,0,0,0} < Block(h1)(1)@124a1c0b8 next=124a1c2b8 {0,0,0,0} wm=v-rl-ltr: logical size={0,0} parent wm=v-rl-ltr, cs={0,0}, logicalRect={0,0,0,0} [state=000b120000100602] [content=1247c5e90] [sc=1248fa718]< line 124a1c268: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x1] {0,0,0,0} < Text(0)"\u7f85\u751f\u9580"@124a1c1f8 {0,0,0,0} wm=v-rl-ltr: logical size={0,0} parent wm=v-rl-ltr, cs={0,0}, logicalRect={0,0,0,0} [state=0009000000000402] [content=1247c5f20] [sc=124a1c150:-moz-text] [run=0][0,3,T]

Jonathan Kew [:jfkthame]

Assignee

Comment 14

•

8 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)

Daniel Holbert [:dholbert]

Comment 15

•

8 years ago

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).

Mats Palmgren (inactive)

Comment 16

•

8 years ago

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.