Closed Bug 1227327 Opened 4 years ago Closed 4 years ago

Toggle languages is broken on www.interglot.com

Categories

(Core :: Layout, defect)

45 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox44 --- unaffected
firefox45 + unaffected
firefox46 + unaffected
firefox47 - unaffected
firefox48 + verified
firefox49 --- verified

People

(Reporter: alice0775, Assigned: mstange)

References

()

Details

(Keywords: regression, site-compat)

Attachments

(8 files)

[Tracking Requested - why for this release]:

Steps to reproduce:
1.Open http://www.interglot.com/dictionary/en/fr/search?q=+bonjour&m=key
2. Wait page load
3. Click a icon of yellow arrow which is located between flags

Actual Results:
Nothing happens

Expected Results:
Flags should be exchanged

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=097cac1912b9658ba68389b5dd72d4cc45e762d5&tochange=21cabf6ab3e9

Suspect: Bug 1201327
Flags: needinfo?(mstange)
I can reproduce this.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Flags: needinfo?(mstange)
Attached file testcase
There are lots of things that render CSS background images without the use of nsDisplayBackgroundImage. But I only added the dynamic change handling to nsDisplayBackgroundImage, so now I need to fix up those other display item types.
I'm going to have to back out bug 1201327. For DLBI to work, there needs to be one display item per background layer, and that just isn't true for things like <button>, <td> or <fieldset> - those render all background layers with one call to nsCSSRendering::PaintBackground. I suppose I could add a change hint that invalidates those special frame types and only SchedulePaints the other ones, but that feels very hacky.

Matt, do you have any other ideas?
Flags: needinfo?(matt.woodrow)
The other obvious option is to change the rendering of those items so that they have separate display items too, but that seems like a lot of work.

We already have code that varies invalidation behaviour based on what display items were present last paint [1], so your plan doesn't seem too horrible.

I guess it just depends on how much we think we can win from keeping this optimization.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/ImageLoader.cpp#355
Flags: needinfo?(matt.woodrow)
Tracking because it is a regression.
Markus, do you have news about this issue? THanks
Duplicate of this bug: 1240690
Tracking for 46 and 47, recent regression.  

Note from bug 1420690 that this affects icons on Vimeo so we should check the test case there when this is fixed.
Flags: qe-verify+
Depends on: 1244258
I won't be able to fix this in time for 45. I've filed bug 1244258 for backing out bug 1201327 from 45.
Updating the tracking flags accordingly.
Markus, do you have an eta for a fix for 46? thanks
Flags: needinfo?(mstange)
Hopefully before the end of this week.
Flags: needinfo?(mstange)
Bug 1201327 got backed out on 46, so this is now 46:unaffected.
Hi Markus, this is a tracked bug for Fx47, just wondering if we have a fix in the works.
Flags: needinfo?(mstange)
Unfortunately we'll need to back out bug 1201327 again. I still haven't had a chance to write a proper fix.
I'm currently waiting for try results on that 47 backout.
Flags: needinfo?(mstange)
I've backed out bug 1201327 on Aurora 47, so this bug is only present on Nightly now.
I have patches now. I'm currently running them to try. I still need to write tests.
Theoretically we should do the same for nsTreeBodyFrame, but that frame type is
harder to detect and I'm not sure it's worth adding code to support updating
background-position on XUL trees.

Review commit: https://reviewboard.mozilla.org/r/49513/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49513/
Attachment #8746652 - Flags: review?(cam)
Attachment #8746653 - Flags: review?(matt.woodrow)
Attachment #8746654 - Flags: review?(matt.woodrow)
Attachment #8746655 - Flags: review?(matt.woodrow)
Attachment #8746656 - Flags: review?(matt.woodrow)
Attachment #8746657 - Flags: review?(matt.woodrow)
This has multiple benefits:
 - It makes DLBI detection of background-position changes work for buttons.
 - It makes background-attachment: fixed work for button backgrounds.
 - It allows the willPaintBorder optimization to be used for button background
   drawing, which reduces the background clip to not overlap with opaque borders.

The willPaintBorder optimization requires a change to the reftest 611574-2.html.
This reftest compares backgrounds to inset box shadows. Box shadows those don't
have a willPaintBorder optimization, so we'd get different results due to the
borders - inset box shadows will bleed through the rounded corner anti-aliasing
of the border, backgrounds won't any more. So we just turn off button borders
in that reftest.

Review commit: https://reviewboard.mozilla.org/r/49517/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49517/
Fieldsets break up their border so we need to disable the willPaintBorder optimization for them.

***

Review commit: https://reviewboard.mozilla.org/r/49519/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49519/
Comment on attachment 8746653 [details]
MozReview Request: Bug 1227327 - Allow specifying a background rect for background dislay items. r?mattwoodrow

https://reviewboard.mozilla.org/r/49515/#review46375

::: layout/base/nsDisplayList.cpp:2821
(Diff revision 1)
>    if (frame->GetType() == nsGkAtoms::canvasFrame) {
>      nsCanvasFrame* canvasFrame = static_cast<nsCanvasFrame*>(frame);
>      clipRect = canvasFrame->CanvasArea() + aItem->ToReferenceFrame();

Side track, shouldn't this branch just be an overload on nsDisplayCanvasBackgroundImage?
Attachment #8746653 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8746654 [details]
MozReview Request: Bug 1227327 - Use regular background image display items for painting button backgrounds. r?mattwoodrow

https://reviewboard.mozilla.org/r/49517/#review46377
Attachment #8746654 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8746655 [details]
MozReview Request: Bug 1227327 - Make fieldset frames build nsDisplayBackgroundImage items. r?mattwoodrow

https://reviewboard.mozilla.org/r/49519/#review46379
Attachment #8746655 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8746656 [details]
MozReview Request: Bug 1227327 - Use regular background drawing for XUL groupbox frames. r?mattwoodrow

https://reviewboard.mozilla.org/r/49521/#review46381
Attachment #8746656 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8746657 [details]
MozReview Request: Bug 1227327 - Add tests. r?mattwoodrow

https://reviewboard.mozilla.org/r/49523/#review46383
Attachment #8746657 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8746652 [details]
MozReview Request: Bug 1227327 - Invalidate table parts and MathML frames when background-position changes on them. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49513/diff/1-2/
Attachment #8746652 - Attachment description: MozReview Request: Bug 1227327 - Invalidate table parts and MathML frames when background-position changes on them. r?heycam → MozReview Request: Bug 1227327 - Invalidate table parts and MathML frames when background-position changes on them. r?dbaron
Attachment #8746652 - Flags: review?(cam) → review?(dbaron)
Comment on attachment 8746653 [details]
MozReview Request: Bug 1227327 - Allow specifying a background rect for background dislay items. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49515/diff/1-2/
Comment on attachment 8746654 [details]
MozReview Request: Bug 1227327 - Use regular background image display items for painting button backgrounds. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49517/diff/1-2/
Comment on attachment 8746655 [details]
MozReview Request: Bug 1227327 - Make fieldset frames build nsDisplayBackgroundImage items. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49519/diff/1-2/
Comment on attachment 8746656 [details]
MozReview Request: Bug 1227327 - Use regular background drawing for XUL groupbox frames. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49521/diff/1-2/
Comment on attachment 8746657 [details]
MozReview Request: Bug 1227327 - Add tests. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49523/diff/1-2/
Comment on attachment 8746652 [details]
MozReview Request: Bug 1227327 - Invalidate table parts and MathML frames when background-position changes on them. r?dbaron

https://reviewboard.mozilla.org/r/49513/#review46847
Attachment #8746652 - Flags: review?(dbaron) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3b5d1530f701 for failures in test_fixed_bg_scrolling_repaints.html like https://treeherder.mozilla.org/logviewer.html#?job_id=27073246&repo=mozilla-inbound across what looks like every platform.
FYI this resulted in ~15% regression on tscrollx on MacOS X before it was backed out (and a ~9% improvement on Linux):

https://treeherder.mozilla.org/perf.html#/alerts?id=1077

It would be nice to see if we could mitigate the Mac regression before relanding.
There was a bug in the second patch that caused canvas background image items to be in the wrong place. With that fixed there shouldn't be any test failures or Talos regressions any more.
(The constructor for nsDisplayCanvasBackgroundImage failed to add the ToReferenceFrame offset to the aBackgroundRect argument that it was passing to the nsDisplayBackgroundImage constructor.)
Comment on attachment 8746653 [details]
MozReview Request: Bug 1227327 - Allow specifying a background rect for background dislay items. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49515/diff/2-3/
Comment on attachment 8746654 [details]
MozReview Request: Bug 1227327 - Use regular background image display items for painting button backgrounds. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49517/diff/2-3/
Comment on attachment 8746655 [details]
MozReview Request: Bug 1227327 - Make fieldset frames build nsDisplayBackgroundImage items. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49519/diff/2-3/
Comment on attachment 8746656 [details]
MozReview Request: Bug 1227327 - Use regular background drawing for XUL groupbox frames. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49521/diff/2-3/
Comment on attachment 8746657 [details]
MozReview Request: Bug 1227327 - Add tests. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49523/diff/2-3/
I'm going to mark layout/reftests/bugs/942672-1.html as fuzzy (1,1) and fix the new tests to specify border-radius:0 on input elements because Fennec's default stylesheets sets a non-zero border radius on them. This happens at https://dxr.mozilla.org/mozilla-central/rev/0a25833062a880f369e6f9f622413a94cc671bf4/mobile/android/themes/core/content.css#103 .
I've also found one more place where the second patch needs to use mBackgroundRect instead of the frame's size.
Comment on attachment 8746653 [details]
MozReview Request: Bug 1227327 - Allow specifying a background rect for background dislay items. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49515/diff/3-4/
Comment on attachment 8746654 [details]
MozReview Request: Bug 1227327 - Use regular background image display items for painting button backgrounds. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49517/diff/3-4/
Comment on attachment 8746655 [details]
MozReview Request: Bug 1227327 - Make fieldset frames build nsDisplayBackgroundImage items. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49519/diff/3-4/
Comment on attachment 8746656 [details]
MozReview Request: Bug 1227327 - Use regular background drawing for XUL groupbox frames. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49521/diff/3-4/
Comment on attachment 8746657 [details]
MozReview Request: Bug 1227327 - Add tests. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49523/diff/3-4/
(In reply to Philip Chee from comment #53)
That sounds like it's from bug 1267524 and not from this bug.
Duplicate of this bug: 1227012
Duplicate of this bug: 1239856
Duplicate of this bug: 1245255
Comment on attachment 8746652 [details]
MozReview Request: Bug 1227327 - Invalidate table parts and MathML frames when background-position changes on them. r?dbaron

I'm requesting uplift to 48 for all patches of this bug.

Approval Request Comment
[Feature/regressing bug #]: regression from bug 1201327
[User impact if declined]: buttons sometimes fail to update their visuals
[Describe test coverage new/current, TreeHerder]: these patches add tests
[Risks and why]: moderately low, has baked for more than a week
[String/UUID change made/needed]: none
Attachment #8746652 - Flags: approval-mozilla-aurora?
Because we are in the middle of the aurora phase, patches are big, and it's a minor performance improvement, can we try to let it ride the train of 49? Can we instead back out the patch of 1201327 in 48?
Flags: needinfo?(mstange)
Okay, we can do that instead. Instead of backing out all of those patches (which would be hard because the code has changed in the meantime), this patch just backs out the crucial bit.

Approval Request Comment
[Feature/regressing bug #]: regression from bug 1201327
[User impact if declined]: buttons sometimes fail to update their visuals
[Describe test coverage new/current, TreeHerder]: none. I guess I could include the tests from this bug in this backout patch, not sure if it's worth it.
[Risks and why]: very low, restores old behavior
[String/UUID change made/needed]: none
Flags: needinfo?(mstange)
Attachment #8752960 - Flags: approval-mozilla-aurora?
Attachment #8746652 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1261292
Comment on attachment 8752960 [details] [diff] [review]
small backout patch for Aurora 48

Fix a regression, taking it
Attachment #8752960 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed FX 48b1, 49.0a2 (2016-06-14) Win 7
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1248841
Depends on: 1295111
You need to log in before you can comment on or make changes to this bug.