Closed Bug 817406 Opened 12 years ago Closed 10 years ago

'direction' isn't propagated to the canvas

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: kennyluck, Assigned: MatsPalmgren_bugz)

References

(
URL
)

Details

Attachments

(5 files, 2 obsolete files)

Let ApplyStyleFixups fix up the 'direction' of a root frame
4.86 KB, patch
bzbarsky
: feedback+
Details | Diff | Splinter Review
Get rid of the 'direction' propogation logic in nsGfxScrollFrameInner::IsLTR()
5.00 KB, patch
Details | Diff | Splinter Review
fix v3
6.84 KB, patch
heycam
: review+
Details | Diff | Splinter Review
more reftests
8.78 KB, patch
Details | Diff | Splinter Review
ProcessOneRestyle version
14.51 KB, patch
Details | Diff | Splinter Review
It's somehow suprising (to me) but Firefox seems to be the only browser that doesn't propagate 'direction' to the canvas/initial containing block. The relevant spec prose is this from CSS 2.1 10.1 Definition of "containing block"[1]: # The 'direction' property of the initial containing block is the same as # for the root element. (CSS 2.1 doesn't have a test like this? Really?) [1] http://www.w3.org/TR/CSS2/visudet.html#containing-block-details
This seems somewhat hackish and since it depends on bug 738190 to pass /layout/reftests/bidi/229367-2.html and /layout/reftests/bidi/263359-2.html, if I guess correctly, I'll just request feedback for now. One of the test case is a bit contravertial, see below.
Assignee: nobody → kennyluck
Attachment #687540 - Flags: feedback?(bzbarsky)
So 817406-5.html relies on not propagating 'direction' from <body> to the viewport scroll frame, a backwards- and IE10-imcompatibile change with which 379461-3-container-xhtml.html has to be removed. 817406-4.html is simiarly imcompatible. However, this is what the spec requires and it was discussed[1] but didn't lead to a spec change. Would it be safer to propagate also the direction on <body> to the viewport? But then what's described in [2] seems to be depending on the existence of the 'dir' attribute and sounds ... ugly. I need more testing here. [1] http://lists.w3.org/Archives/Public/www-style/2010Oct/0750 [2] https://bugs.webkit.org/show_bug.cgi?id=48157
Attachment #687545 - Flags: feedback?(bzbarsky)
Depends on: 738190
Comment on attachment 687540 [details] [diff] [review] Let ApplyStyleFixups fix up the 'direction' of a root frame Kenny, mind posting a patch with 8 lines of context, not 3, and with -p? Those are much easier to make sense of...
Comment on attachment 687540 [details] [diff] [review] Let ApplyStyleFixups fix up the 'direction' of a root frame The "if (mPseudoTag)" test should really test for the correct pseudo-tag, no? Doing that for all pseudo styles is totally wrong. The comment about updating "to the root frame" should talk about "of the root frame", right? Other than that, I guess this is OK. Might be good to have roc or dbaron review this, though.
Attachment #687540 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 687545 [details] [diff] [review] Get rid of the 'direction' propogation logic in nsGfxScrollFrameInner::IsLTR() I'm worried about compat issues here. Propagating from body seems like something authors would depend on.... What do other UAs do?
Attachment #687545 - Flags: feedback?(bzbarsky)
Attached patch fix v2 (obsolete) — — Splinter Review
I updated Kenny's patch to trunk and addressed bz's point about checking the correct pseudo-tag in comment 4. Note that I excluded the nsCSSFrameConstructor change from the original patch, because I don't grok what that part is trying to do, and all reftests in layout/reftests/bidi/reftest.list pass without it (including the ones added here). Perhaps that part was only needed b/c of the missing pseudo check? (I checked that this patch also fixes bug 1161752)
Boris, do you recall what the nsCSSFrameConstructor change in Kenny's original patch is needed for?
Flags: needinfo?(bzbarsky)
Does the patch without that change handle the root element being removed and a different one, with a different direction, inserted?
Flags: needinfo?(bzbarsky)
(In reply to Not doing reviews right now from comment #9) > Does the patch without that change handle the root element being removed and > a different one, with a different direction, inserted? Yes, it appears so. Using the test in bug 1161752, when I remove the inner document's <html>, clone it, set dir=rtl on the clone, insert the clone, it gives the expected result: going from ltr=>rtl is a PASS and rtl=>ltr a FAIL. I also tried just toggling display:none/block and that works too.
Then it's not obvious to me why the CSS frame constructor change is needed... Oh, I assume that changing the direction style on the root without inserting/removing it works too?
How does this relate to bug 1071098, and do the review comments there apply here? (I didn't investigate more deeply than reading the titles of the bugs; don't have time right now.)
(In reply to Kang-Hao (Kenny) Lu [:kennyluck] from comment #0) > (CSS 2.1 doesn't have a test like this? Really?) > > [1] http://www.w3.org/TR/CSS2/visudet.html#containing-block-details You are correct: there is *_no_* test in CSS 2.1 test suite specifically testing this statement: "The 'direction' property of the initial containing block is the same as for the root element." and there definitely should be. ------- (In reply to Not doing reviews right now from comment #5) > I'm worried about compat issues here. Propagating from body seems like > something authors would depend on.... What do other UAs do? Issue 758073003: Always propagate direction and writing-mode from <body> https://codereview.chromium.org/758073003 But I believe (I could be wrong on this) propagating 'direction' from <body> to ICB has not been decided yet at CSSWG. There is a long thread on this in www-style mailing list with subject line with many posts in march 2015: "[css-writing-modes][CSS21] propagation of 'direction' from <body>" which started here http://lists.w3.org/Archives/Public/www-style/2014Sep/0303.html with the last one being http://lists.w3.org/Archives/Public/www-style/2015Mar/0393.html and with http://lists.w3.org/Archives/Public/www-style/2015Feb/0261.html proposing 3 options: " There are three options for what to do: 1) require 'direction' propagation from body to html, fix browsers 2) forbid 'direction' propagation from body to html, fix browsers 3) require dir=rtl propagation from body to html, fix browsers "
(In reply to Not doing reviews right now from comment #11) > Oh, I assume that changing the direction style on the root without > inserting/removing it works too? Yes. I'll try to wrap up these dynamic tests into a reftest.
(In reply to David Baron [:dbaron] ⏰UTC+2 (busy, returning May 21) from comment #12) > How does this relate to bug 1071098, and do the review comments there apply > here? Re: bug 1071098 comment 16: Regarding ScrollFrameHelper::IsLTR, FYI, Boris was worried about compat issues in comment 5. I think we should leave that as is and investigate in a follow-up bug if we need to change that somehow. Regarding your points (1)(2)(3), I think those only apply if we do it like the patch in that bug, i.e. not propagating the style but instead peeking at some other frame's style. I suspect those points are moot with this patch since the ICB has the right style value? Re: 'writing-mode' - it makes sense to me to also propagate that value from the root element to the ICB in the same way this patch does for 'direction'. Let me know if you want me to write that up. Wether or not we should also propagate some values from body to html (and from there to the ICB) seems like a separate issue that we can investigate in a follow-up bug.
(In reply to Mats Palmgren (:mats) from comment #15) > Re: 'writing-mode' - it makes sense to me to also propagate > that value from the root element to the ICB in the same way > this patch does for 'direction'. Let me know if you want me > to write that up. " The principal writing mode of the document is determined by the writing-mode and direction values specified on the root element. " 3.1 Block Flow Direction: the writing-mode property http://www.w3.org/TR/css-writing-modes-3/#writing-mode I believe this issue (propage 'writing-mode' value from document root element to ICB) should go into a separate, distinct bug report. This issue is mixed with bug 1102175 right now. > Wether or not we should also propagate some values from body > to html (and from there to the ICB) seems like a separate > issue that we can investigate in a follow-up bug. Agree. This is bug 1102175. I think CSSWG should first give the OK on this. Just my opinion.
(In reply to Gérard Talbot from comment #17) > I believe this issue (propage 'writing-mode' value from document root > element to ICB) should go into a separate, distinct bug report. This issue > is mixed with bug 1102175 right now. Wasn't that already fixed in bug 1102175? (per bug 1102175 comment 19). (The remaining part is propagating it from <body>, AFAICT.) Are you saying there are cases where it's set on <html> that we don't layout correctly?
(In reply to Mats Palmgren (:mats) from comment #18) > (In reply to Gérard Talbot from comment #17) > > I believe this issue (propage 'writing-mode' value from document root > > element to ICB) should go into a separate, distinct bug report. This issue > > is mixed with bug 1102175 right now. > > Wasn't that already fixed in bug 1102175? (per bug 1102175 comment 19). Yes, it (propagation of 'writing-mode' value from document root element to ICB)) was fixed in bug 1102175. Both http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s31-block-flow-direction-025.xht and http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/wm-block-flow-direction-002-root.xht pass in Firefox 40.0a1 (2015-04-30). > (The remaining part is propagating it from <body>, AFAICT.) > Are you saying there are cases where it's set on <html> that we don't > layout correctly? No. I did *not* want to suggest that there were cases where it's set on <html> that we don't layout correctly. Sorry for the confusion.
OK, cool. So that leaves if/how to propagate style from <body> for both 'direction' and 'writing-mode' to be resolved in follow-up bugs. Bug 1102175 is open for 'writing-mode' and I'll file a separate one for 'direction'. (It's probably a good idea to coordinate those two and land them close together though.)
Comment on attachment 8601815 [details] [diff] [review] fix v2 bz said "I guess this is OK" in comment 4, fwiw. Changes from the original patch: * dropped the nsCSSFrameConstructor changes since I don't understand what they are needed for * checking explicitly for the canvas pseudo-tags to address bz's request in comment 4 * doing a generic "direction != otherDirection" check rather than hard-coding a fix just for RTL The Try results looks OK, although it's hard to tell these days :-/
Attachment #8601815 - Flags: review?(dbaron)
Attachment #8601815 - Flags: review?(dbaron) → review?(roc)
Comment on attachment 8601815 [details] [diff] [review] fix v2 Review of attachment 8601815 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleContext.cpp @@ +604,5 @@ > + presContext->StyleSet()->ResolveStyleFor(docElement, nullptr); > + auto dir = rootStyle->StyleVisibility()->mDirection; > + if (dir != StyleVisibility()->mDirection) { > + nsStyleVisibility* uniqueVisibility = > + (nsStyleVisibility*)GetUniqueStyleData(eStyleStruct_Visibility); Trailing whitespace. I'm not sure this is the most efficient way to do this... Let's ask heycam
Attachment #8601815 - Flags: review?(roc) → review?(cam)
I don't see a better way to handle this. Although it's a bit odd to be looking through up through the pres context to find an element, since all the other fixups we apply are based purely on looking at the style context tree. The ResolveStyleFor shouldn't be that bad, as when we go to actually resolve style for that element, we should get find the same style context that we computed here (in FindChildWithRules). And adding another unique case here isn't terrible, as obviously there's only one of these per document. Do these canvas/scrolledCanvas frames have their mContent set to the root element? Can we add an assertion to make sure that that the canvas/scrolledCanvas frames are indeed for the root element we extract out? Do we need to do anything to handle printing? I see in nsCSSFrameConstructor::ConstructRootFrame that we use pageSequence and scrolledPageSequence there.
(In reply to Cameron McCormack (:heycam) from comment #23) > Do these canvas/scrolledCanvas frames have their mContent set to the root > element? Can we add an assertion to make sure that that the > canvas/scrolledCanvas frames are indeed for the root element we extract out? Yes, except for printing (see below), but the root element doesn't have a frame at this point (at least it's not attached to the content yet). ApplyStyleFixups is called from the nsStyleContext ctor during frame construction, so I don't think we can assert that here. > Do we need to do anything to handle printing? I see in > nsCSSFrameConstructor::ConstructRootFrame that we use pageSequence and > scrolledPageSequence there. Good question, this is what the frame tree looks like in Print Preview: Viewport(-1) [sc=7fbc827670f8:-moz-viewport^0]< HTMLScroll(html)(-1) [content=7fbc81bd07c0] [sc=7fbc82767988:-moz-viewport-scroll]< ... scrollbar frames etc ... SimplePageSequence(html)(-1) [content=7fbc81bd07c0] [sc=7fbc8276e708:-moz-scrolled-page-sequence]< Page(-1) [sc=7fbc8276ec40:-moz-page]< PageContent(-1) [sc=7fbc7d86c2f8:-moz-pagecontent]< Canvas(-1) [sc=7fbc7d86c648:-moz-canvas]< Block(html)(-1) [content=7fbc81bd07c0] [sc=7fbc7d86c8a8^0]< So we have a canvas frame also in this case although it's not actually the ICB in this case, the PageContent frame is. The test for bug 1161752 appears to work fine in Print Preview though. But perhaps not if we change the test to use position:fixed ?
(...) > So we have a canvas frame also in this case although it's not actually > the ICB in this case, the PageContent frame is. The test for bug 1161752 > appears to work fine in Print Preview though. But perhaps not if we change > the test to use position:fixed ? Is the following test with 'position: fixed' instead of 'position: absolute' okay, suitable for you? Test ---- http://www.gtalbot.org/BrowserBugsSection/css21testsuite/fixed-pos-non-replaced-icb-overconstrain-001.xht with the embedded file http://www.gtalbot.org/BrowserBugsSection/css21testsuite/embedded-doc-fixed-pos-non-replaced-icb-overconstrain-001.html Expected result --------------- http://www.gtalbot.org/BrowserBugsSection/css21testsuite/abs-pos-non-replaced-icb-overconstrain-001-ref.xht
Here a single-file test (with 'position: fixed'), with proportional values only so that it will work regardless of width of viewport: http://www.gtalbot.org/BrowserBugsSection/css21testsuite/fixed-pos-non-replaced-icb-overconstrain-009.xht Such test is CSS3 compliant and *not* CSS2.1 compliant. I have printed it with Firefox 37.0.2 and the test failed when printed. IE11 and Chrome 42.0.2311.152 pass the test when viewing on screen. I think the test failed when trying to print the test with Chrome 42.0.2311.152. I have not tried to print the test with IE11. Hope this test helps you. Gérard
I have added media="screen, print" to style blocks of all my related tests... just to be sure, just in case such omission could or would interfere with print preview of tests.
Thanks Gérard, that helps a lot. It shows that the earlier patch is insufficient.
Attached patch fix v3 — — Splinter Review
The ViewportFrame is the containing block for fixed-pos so the earlier patch didn't work since it only fixed the CanvasFrame style context. So I think we can tweak just the viewport style, since 'direction' is inherited by default it will be picked up by the CanvasFrame/PageContentFrame. As far as I can tell this works for abs/fixed pos in both screen and print modes. The additional change to ConstructDocElementFrame is to restyle also the viewport frame. It appears we normally reconstruct frames from the CanvasFrame down, so at this point the frame tree consists of the viewport frame alone.
Attachment #8601815 - Attachment is obsolete: true
Attachment #8601815 - Flags: review?(cam)
Attachment #8606742 - Flags: review?(cam)
Comment on attachment 8606742 [details] [diff] [review] fix v3 Review of attachment 8606742 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSFrameConstructor.cpp @@ +2401,5 @@ > + { > + nsRefPtr<nsStyleContext> sc = mPresShell->StyleSet()-> > + ResolveAnonymousBoxStyle(nsCSSAnonBoxes::viewport, nullptr); > + GetRootFrame()->SetStyleContextWithoutNotification(sc); > + } Does this mean we'll always resolve style twice for the root frame? (Once for when we resolve its initial style and then a second time in here when we get to constructing the root element's frame?) I guess it's not too terrible; it'll only happen the first time. Is it possible that we will miss some change hint processing because we don't restyle the root frame through the normal restyle process -- or can we never actually get different styles (other than the direction propagation) outside of UA style sheets? Would it be better to handle this in RestyleTracker::ProcessOneRestyle by checking whether aElement is the root element, and if so, to set primaryFrame to the viewport frame and start restyling from there? ::: layout/style/nsStyleContext.cpp @@ +603,5 @@ > + nsRefPtr<nsStyleContext> rootStyle = > + presContext->StyleSet()->ResolveStyleFor(docElement, nullptr); > + auto dir = rootStyle->StyleVisibility()->mDirection; > + if (dir != StyleVisibility()->mDirection) { > + nsStyleVisibility* uniqueVisibility = Trailing space.
CSS WG is making decisions right now about what should happen here; proposal is that the HTML attribute on the body propagate to html (resolving dir=auto against the body's contents), but not CSS styles on body. Not sure how related that is to what this patch does (in the middle of WG meeting right now). But it's not 100% sure that we'll be able to do that Web-compat-wise, and if it fails the idea would be to go back to propagating CSS direction:rtl from body to html.
Thanks for the info. We decided to spawn that off to a follow-up bug. So this bug is just about the html -> viewport propagation.
(In reply to Cameron McCormack (:heycam) from comment #31) > Does this mean we'll always resolve style twice for the root frame? Yes, I don't see a way around that I'm afraid. > Is it possible that we will miss some change hint processing because we > don't restyle the root frame through the normal restyle process -- or can we > never actually get different styles (other than the direction propagation) > outside of UA style sheets? I'm not sure I follow. Only 'direction' affects the viewport AFAICT from ApplyStyleFixups. There is also something about scrolling in: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=f58aab6a4e62#2323 and http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=530555a2d6d4#4190 I figured we could tackle those in the follow-up bug when we know how to handle propagation from <body>. (We might want to extend the ApplyStyleFixups to 'writing-mode' there since that appears to influence viewport scrollbars.) > Would it be better to handle this in RestyleTracker::ProcessOneRestyle by > checking whether aElement is the root element, and if so, to set > primaryFrame to the viewport frame and start restyling from there? I tried that, but it fails several of the tests added here. I suspect that we don't get to ProcessOneRestyle in all cases, perhaps not when we initially construct the frame tree? Both 'direction' and 'writing-mode' conveniently causes frame construction so I'm pretty sure ConstructDocElementFrame would catch changes to those properties. http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp?rev=7bc6ca149561#2948
The ProcessOneRestyle version, fwiw. I tried a few variations but couldn't get it to pass the tests in this bug.
Yeah, I'm satisfied that ConstructDocElementFrame will catch the cases we care about because of the frame reconstruction hint generated by these properties. I just thought the solution might be simpler (and would let us avoid the wasted style context resolution) if handling it in ProcessOneRestyle. Not sure why that approach doesn't work. (In reply to Mats Palmgren (:mats) from comment #34) > (In reply to Cameron McCormack (:heycam) from comment #31) > > Is it possible that we will miss some change hint processing because we > > don't restyle the root frame through the normal restyle process -- or can we > > never actually get different styles (other than the direction propagation) > > outside of UA style sheets? > > I'm not sure I follow. Only 'direction' affects the viewport AFAICT from > ApplyStyleFixups. There is also something about scrolling in: > http://mxr.mozilla.org/mozilla-central/source/layout/base/ > nsCSSFrameConstructor.cpp?rev=f58aab6a4e62#2323 > and > http://mxr.mozilla.org/mozilla-central/source/layout/generic/ > nsGfxScrollFrame.cpp?rev=530555a2d6d4#4190 > > I figured we could tackle those in the follow-up bug when > we know how to handle propagation from <body>. OK.
Comment on attachment 8606742 [details] [diff] [review] fix v3 r=me with the trailing space mentioned earlier removed.
Attachment #8606742 - Flags: review?(cam) → review+
Filed bug 1169065 about propagating @dir and/or 'direction' from body.
Assignee: kennyluck → mats
Flags: in-testsuite+
I'm not sure what to make of those reftest failures. It looks like all of them except the last is just a few pixels around the edges of the scrollbar thumb. Are these scrollbars "fade-away"? If so, it might just be that the tests are timing dependent? Timothy, these tests are from bug 1133905. Do you see an actual error in the reftest results here? http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-inbound-emulator/1432765154/mozilla-inbound_ubuntu64_vm-b2g-emulator_test-reftest-7-bm115-tests1-linux64-build349.txt.gz&only_show_unexpected=1
Flags: needinfo?(tnikkel)
The tests are only intended to match with fuzz so I'm fine with adjusting the fuzz on those tests to make them pass. But I'm curious how the patches in this bug changed the painted result for those reftests (I haven't looked at the patches).
Flags: needinfo?(tnikkel)
Yeah, I'm curious too. The patch propagates the 'direction' style from the root element style to the style context of the ViewportFrame. I wasn't really expecting it would affect scrollbars at all, since those are handled manually in: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=f58aab6a4e62#2323 and http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=530555a2d6d4#4190 I'm not worried about the five first failures, those can be fixed with some additional fuzzing. It's the horizontal scrollbar diff in the last test that's odd. It looks like it's the *reference* is missing it. I'm not sure if this is just due to timing or what. I tried this in a b2g-desktop build with user_pref('layers.async-pan-zoom.enabled',true); but that fails several of these tests without any changes at all. I think I'll submit a Try run with about:blank as reference just to see what these tests should look like normally...
Here's the about:blank Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=334d9be2a920 The only significant difference between the test snapshots here to the test snapshots in the failed run is that the vertical scrollbar in the top-left corner is a few pixels more to the left. That diff isn't what the original failures were about though so I'm assuming this is irrelevant (perhaps the viewport width or scaling is slightly different between devices?). I think I'll just increase the fuzz factors to make these tests pass. Timothy, let me know if you see something that I missed or if you have better ideas.
(In reply to Mats Palmgren (:mats) from comment #43) > I'm not worried about the five first failures, those can be fixed with some > additional fuzzing. It's the horizontal scrollbar diff in the last test > that's odd. It looks like it's the *reference* is missing it. I'm not sure > if this is just due to timing or what. Oh, I only noticed the vertical scrollbar differences. That is odd. Oh, I forgot to mention, reftests use the pref layout.testing.overlay-scrollbars.always-visible so that overlay scrollbars dont fade to avoid fading issues.
(In reply to Timothy Nikkel (:tn) from comment #45) > Oh, I only noticed the vertical scrollbar differences. That is odd. Yeah, the 1133905-5-vh-rtl.html test is using the same reference as the other *-vh-rtl.html tests although it clearly has a horizontal scrollbar that the other tests (and refeference) don't have. It's got a huge fuzz count though that masks it. Given that this is unrelated to my changes (as you can see in the "about:blank" test run above) I bumped the fuzz counts and landed my changes. > reftests use ... to avoid fading issues. Ah, good to know, thanks.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: