Closed Bug 518172 Opened 11 years ago Closed 10 years ago
-moz-transform doesn't rotate individual characters in this testcase, except when text is partially repainted
280 bytes, text/html
279 bytes, text/html
276 bytes, text/html
263 bytes, text/html
634 bytes, patch
|Details | Diff | Splinter Review|
STEPS TO REPRODUCE: 1. Load testcase. 2. Then try selecting a sub-section of the string "rotated text". 2. Ctrl+A to select all. EXPECTED RESULTS: The string "rotated text" (including each of its characters) should be fully rotated 90 degrees, the whole time. ACTUAL RESULTS: - At pageload, the *string as a whole* is rotated, but the individual characters are not. - Selecting portions of the string (step 2) makes the whole string flip to the correct orientation, though the un-rotated "r" at the top of the string doesn't get repainted (because it sticks out of the repainted box) - Select-all re-breaks everything -- i.e. it puts the characters back in their un-rotated orientation, as they were at pageload. BROKEN: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090922 Minefield/3.7a1pre WORKING: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:18.104.22.168) Gecko/20090715 Firefox/3.5.1 So, this is a regression w.r.t. the 1.9.1 branch. (Note: I first noticed this bug on bug 467297's testcase)
Requesting blocking, since this is a layout regression from 1.9.1.
Here's a semi-reference case -- I just removed the 'a' character in the second table cell. This shows expected behavior.
That is weird. Shouldn't be hard to track down there.
Looks like a very recent regression: WORKS: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090914 Minefield/3.7a1pre BROKEN: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090915 Minefield/3.7a1pre Regression pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=912c6ae3b70c&tochange=cdcf1519121f I'm blaming the cairo update, bug 515192 / changeset 7a9262f6dbc0.
(In reply to comment #1) > Requesting blocking, since this is a layout regression from 1.9.1. Switching my earlier blocking request from 1.9.2 to 1.9.3, since this doesn't actually affect 1.9.2. (The regressing bug didn't land on that branch, and I've verified that this doesn't affect the latest 1.9.2 nightly.)
blocking2.0: --- → ?
If it's a regression from a cairo update, we should probably move it to the Graphics component.
Component: Layout → Graphics
QA Contact: layout → thebes
blocking2.0: ? → alpha1
fwiw, -moz-scale is also broken for text: - text is not scaled (but as above, when selected it is suddenly scaled) - bad kerning & letter-spacing between characters (some have the spacing as if it was scaled, some not) (and I see that on both OS X and Linux)
OS: Linux → All
Summary: -moz-transform doesn't rotate individual characters in this testcase, except when a portion is selected → -moz-transform doesn't rotate individual characters in this testcase, except when text is partially repainted
I landed reftests for this bug: http://hg.mozilla.org/mozilla-central/rev/354735881fa8 with one set (without the extra character required to cause the bug) passing and the other set failing. So if this gets magically fixed by another cairo upgrade, somebody will know to come update the bug. And also so we have tests for it.
Changing blocking1.9.3: from alpha1 to beta1 per conversation with Joe Drew and Jeff Muizelaar. Still good to fix sooner rather than later, though.
blocking2.0: alpha1 → beta1
Bug 551405 (mozTextAlongPath stops rotating along mozPathText) is the same visual glitch but occurs in Vladimir Vukićević/roc's HTML canvas demo rather than CSS; is it a duplicate of this bug?
Just noted on Bug 551405 that these two bugs may have the same root cause, but for now, it's worth leaving them separate since the paths to reproducing them (the testcases) are so different.
This looks like the ctm isn't getting propagated to the scaled font. I'm not sure who's responsibility this is.
Using an SVG test case for character rotation, it fails when displayed normally but works when rendered through drawWindow (In my case, a tab preview extension) http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-text-text-07-t.html The testcase attached to this bug is no different.
The comment 15 testcase also works if you display the frame in a tab.
This blocks _a_ beta, but not necessarily beta 1.
blocking2.0: beta1+ → beta2+
Another test case: http://disruptive-innovations.com/zoo/rotator/rotator.xul
I'd note that playing with various examples of animation of transforms shows that this seems to happen only for some transformations (those at right angles) -- a bunch of the animations in http://dbaron.org/css/test/2010/transition-negative-determinant (such as the middle two in the top row) animate correctly through the animation, but then flip to the incorrect result after the animation completes. Some of the ones in the bottom row have the opposite behavior, though, so maybe that explanation doesn't particularly make sense.
We've hit this bug on the "What's new" page for Beta 2. (bug 578138)
The bug goes away if I comment out these two lines in cairo_set_scaled_font (which were added in the regressing changeset): if (was_previous) →·······cr->gstate->scaled_font = cairo_scaled_font_reference ((cairo_scaled_font_t *) scaled_font);
There are various obvious patches that make the bug go away, but I'm really having trouble understanding what the code is *supposed* to be doing -- what invariants previous_scaled_font and scaled_font should maintain, whether _cairo_gstate_unset_scaled_font is what's supposed to be called when those invariants change, etc.
We should fix this before final. It's a pretty bad regression.
e.g. for beta3.
blocking2.0: final+ → ?
(And by "various obvious patches" I really mean "various patches that end up disabling the previous_scaled_font cache entirely".)
The upstream commit: commit 7a023a62f7517ad0d54f4d59c99909fadcc05e82 Author: Nicolaus L Helper <firstname.lastname@example.org> Date: Thu Jun 17 08:56:30 2010 +0100 ft,fc,xlib: LCD filtering patch. This adds internal API to retrieve the LCD filtering parameters from fontconfig, or as set on the Screen, and feed them to FreeType when rendering the glyph. References: Bug 10301 - LCD filtering patch https://bugs.freedesktop.org/show_bug.cgi?id=10301 Tested-by: Brandon Wright <email@example.com> Forward-ported-by: Robert Hooker <firstname.lastname@example.org> ickle: The API is clearly not ready for public consumption, the enum are poorly named, however this stands by itself as enabling system wide properties. seems to fix the problem. But it's not at all clear why. The best theory I have so far is that it interacts with font hashing. I'll try to figure out what broke it in the first place next.
The commit that caused the regression is: commit dc083ab30a5b781e205354c525ee054982364abd Author: Chris Wilson <email@example.com> Date: Wed May 27 14:54:34 2009 +0100 [cairo] Track the MRU scaled font When observing applications two patterns emerge. The first is due to Pango, which wraps each glyph run within a context save/restore. This causes the scaled font to be evicted after every run and reloaded on the next. This is caught by the MRU slot on the cairo_scaled_font_map and prevents a relatively costly traversal of the hash table and holdovers. The second pattern is by applications that directly manage the rendering of their own glyphs. The prime example of this is gnome-terminal/vte. Here the application frequently alternates between a few scaled fonts - which requires a hash table retrieval every time. By introducing a MRU slot on the gstate we are able to directly recover the scaled font around 90% of the time. Of 110,000 set-scaled-fonts: 4,000 were setting the current font 96,000 were setting to the previous font 2,500 were recovered from the MRU on the cairo_scaled_font_map 7,500 needed a hash retrieval which compares to ~106,000 hash lookups without the additional MRU slot on the gstate. This translates to an elapsed time saving of ~5% when replaying a gnome-terminal trace using the drm backend.
We seem to be calling with cairo_set_scaled_font() with a font that doesn't have a ctm that matches the context. Which is wrong according to the documentation: "Except for some translation, the current CTM of the cairo_t should be the same as that of the cairo_scaled_font_t, which can be accessed using cairo_scaled_font_get_ctm()." This used to work because changing the ctm would cause the scaled_font to be NULLed out and we would recreate the scaled font as needed. When the MRU cache was added we end up actually using the scaled font with the wrong CTM. As an aside, does anyone know the goal of the whole scaled_font/font distinction? CoreGraphics and DWrite don't seem to have a similar concept.
We seem to always create scaled_fonts with their ctm set to identity. That seems wrong...
The setting up of scaled_fonts is actually platform-specific code --- see gfxMacFonts, gfxPangoFonts/gfxFT2FontBase, gfxDWriteFonts, gfxGDIFont. My understanding of scaled_fonts is that by fixing the CTM you'd get a certain set of grid-fitted glyph outlines and metrics that are optimized assuming the destination would have the given CTM, but that you'd be able to render with any CTM if you had to. By that interpretation the documentation you quote is a "should" and not a "must". So we need to clarify with upstream whether it's a "should" or a "must". If it's a "must", then we need to change our font implementations to recreate the scaled font when a new CTM is seen.
(In reply to comment #34) > If it's a "must", then we need to change our font implementations to recreate > the scaled font when a new CTM is seen. If we choose not to fix this in cairo, a possible work-around on our side is to use cairo_set_font_face, cairo_set_font_matrix, and cairo_set_font_options.
My understanding of cairo_scaled_font_t is that it "Replaces the current font face, font matrix, and font options in the #cairo_t with those of the #cairo_scaled_font_t." Whether the scaled font is actually used is an implementation detail and the gstate should only use the provided scaled font if the ctm is suitable. I wonder how much the optimization in comment 31 would have been necessary if https://bugs.freedesktop.org/show_bug.cgi?id=17399 (bug 453200) were fixed.
I think we should fix this as soon as possible so this patch disables the previous_scaled_font cache. I've filed bug #583035 about fixing/figuring out this problem properly.
Attachment #461294 - Flags: review?(karlt)
Comment on attachment 461294 [details] [diff] [review] Disable the previous_scaled_font cache Will be interesting to see whether this affects firefox perf.
Attachment #461294 - Flags: review?(karlt) → review+
http://hg.mozilla.org/mozilla-central/rev/36f921c9692f I'll try to add the reftest today too.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #39) > http://hg.mozilla.org/mozilla-central/rev/36f921c9692f > > I'll try to add the reftest today too. Reftests were already there. They've been set to pass in: http://hg.mozilla.org/mozilla-central/rev/3802d1d04a44
Bug 551405 (mozTextAlongPath stops rotating along mozPathText) now works for me, maybe it was fixed by karlt's check-in; so if it has a reftest than someone can mark that bug FIXED too (yay!).
This does not appear to be fixed. I've posted my usecase and screenshot to another bug that seems to be about this same issue: https://bugzilla.mozilla.org/show_bug.cgi?id=501028 I also asked for people to test my usecase over IRC, and the one person that did try it, got the same broken rendering. Details at the above link. Thanks all, - Jason
Jason: Are the testcases attached on this bug here broken for you? If so, it's likely because you're running iceweasel, and iceweasel might be built with system-cairo rather than Mozilla's bundled cairo. (You can verify this by visiting "about:buildconfig" and looking for mention of "system-cairo" in the build options) The fix to this bug was a fix to cairo code -- so if your Firefox build was built with an external unpatched cairo library, then you'll definitely hit this bug. (This is why Mozilla uses a bundled version of cairo in Firefox.)
(In reply to Daniel Holbert [:dholbert] (vacation 9/17-9/24) from comment #43) > so if your Firefox build was er "if your Iceweasel build" was
(your testcase attached on the other bug renders correctly for me, fwiw (w/ rotated characters), in a firefox Nightly build)
Daniel, Thanks for the explanation of the cairo builtin vs system. Mine is indeed using system cairo. I've made a bug report for the debian cairo maintainers explaining the issue, and linking to this firefox bug report. Looks like the test cases on this bug have the same issues for me as when they were originally reported. The first ("testcase") shows the rotated text funky except when selected with the mouse. The "semi-reference case" works fine. (I too experimented with only having rotated text, and it was fine.) "testcase #2" shows broken, but "reference for #2 shows correctly". All as explained in the comments above. - Jason
Using Firefox 7 stable in Linux. Using this setup: http://pastebin.com/UHKrAf8S I get this bug. If I delete the non-rotated element, the bug goes away. If I change the font-size of either of them, the bug goes away. Keeping the font-size the same, or deleting every style bit except for the transform produces the bug as described by the OP.
> Using Firefox 7 stable in Linux Did you get your browser from mozilla.org? Or from your distro? May be a problem with your Linux distro's Cairo version.
Jordan: your testcase is WORKSFORME using the Mozilla-provided build of Firefox 7, running on Ubuntu x86_64. (First line isn't rotated, second line is.) If you can reproduce the bug you describe w/ Firefox *downloaded directly from Mozilla* at http://getfirefox.com , please open a new bug on it and CC me. > May be a problem with your Linux distro's Cairo version. (indeed -- see comment 43 for more details)
Meh, nevermind. Works in the official one. Don't know why it'd be different in a bleeding edge distro with zero focus on being 100% FOSS, though.
Because at least some Linux distros insist on shipping only one copy of cairo rather than including within firefox the version that we've tested and that we ship.
Well thanks. I took it as an opportunity to switch to UX anyway and its all around much better, no transform bug in sight.
What's the status of the bug upstream?
You need to log in before you can comment on or make changes to this bug.