1227327 - Toggle languages is broken on www.interglot.com

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Alice0775 White]

[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

Comment 1

[Markus Stange [:mstange]]

I can reproduce this.

Comment 2

[Markus Stange [:mstange]]

Attachment: 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.

Comment 3

[Markus Stange [:mstange]]

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?

Comment 4

[Matt Woodrow (:mattwoodrow)]

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

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Tracking because it is a regression.
Markus, do you have news about this issue? THanks

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 8

[Markus Stange [:mstange]]

I won't be able to fix this in time for 45. I've filed bug 1244258 for backing out bug 1201327 from 45.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Updating the tracking flags accordingly.
Markus, do you have an eta for a fix for 46? thanks

Comment 10

[Markus Stange [:mstange]]

Hopefully before the end of this week.

Comment 11

[Markus Stange [:mstange]]

Bug 1201327 got backed out on 46, so this is now 46:unaffected.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Markus, this is a tracked bug for Fx47, just wondering if we have a fix in the works.

Comment 13

[Markus Stange [: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.

Comment 14

[Markus Stange [:mstange]]

I've backed out bug 1201327 on Aurora 47, so this bug is only present on Nightly now.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Markus. Tracked for 48.

Comment 16

[Markus Stange [:mstange]]

I have patches now. I'm currently running them to try. I still need to write tests.

Comment 17

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1227327 - Invalidate table parts and MathML frames when background-position changes on them. r?dbaron

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/

Comment 18

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1227327 - Allow specifying a background rect for background dislay items. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/49515/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49515/

Comment 19

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1227327 - Use regular background image display items for painting button backgrounds. r?mattwoodrow

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/

Comment 20

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1227327 - Make fieldset frames build nsDisplayBackgroundImage items. r?mattwoodrow

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 21

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1227327 - Use regular background drawing for XUL groupbox frames. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/49521/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49521/

Comment 22

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1227327 - Add tests. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/49523/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49523/

Comment 23

[Matt Woodrow (:mattwoodrow)]

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?

Comment 24

[Matt Woodrow (:mattwoodrow)]

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

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8746655 [details]
MozReview Request: Bug 1227327 - Make fieldset frames build nsDisplayBackgroundImage items. r?mattwoodrow

https://reviewboard.mozilla.org/r/49519/#review46379

Comment 26

[Matt Woodrow (:mattwoodrow)]

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

Comment 27

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8746657 [details]
MozReview Request: Bug 1227327 - Add tests. r?mattwoodrow

https://reviewboard.mozilla.org/r/49523/#review46383

Comment 28

[Markus Stange [:mstange]]

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/

Comment 29

[Markus Stange [:mstange]]

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 30

[Markus Stange [:mstange]]

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 31

[Markus Stange [:mstange]]

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 32

[Markus Stange [:mstange]]

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 33

[Markus Stange [:mstange]]

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 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2276fc059bf7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1515512d3731
https://hg.mozilla.org/integration/mozilla-inbound/rev/462dc0904d05
https://hg.mozilla.org/integration/mozilla-inbound/rev/5453b1ce4e85
https://hg.mozilla.org/integration/mozilla-inbound/rev/b726c30c4290
https://hg.mozilla.org/integration/mozilla-inbound/rev/529ff32ced48

Comment 36

[Phil Ringnalda (:philor)]

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.

Comment 37

[William Lachance (:wlach)]

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.

Comment 38

[Markus Stange [:mstange]]

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 39

[Markus Stange [:mstange]]

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 40

[Markus Stange [:mstange]]

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 41

[Markus Stange [:mstange]]

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 42

[Markus Stange [:mstange]]

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 43

[Markus Stange [:mstange]]

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/

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0fe45294034
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7fd04cc749
https://hg.mozilla.org/integration/mozilla-inbound/rev/30e394faeb23
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba58cd94310
https://hg.mozilla.org/integration/mozilla-inbound/rev/c857ad1fa01c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b0ba301426

Comment 45

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b9a2c891fc for Android reftest failures that are probably just the need for fuzz, but I'm not the one to decide that. https://treeherder.mozilla.org/logviewer.html#?job_id=27158167&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=27187811&repo=mozilla-inbound

Comment 46

[Markus Stange [:mstange]]

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 47

[Markus Stange [:mstange]]

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 48

[Markus Stange [:mstange]]

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 49

[Markus Stange [:mstange]]

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 50

[Markus Stange [:mstange]]

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 51

[Markus Stange [:mstange]]

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/

Comment 52

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e83a4f7237e
https://hg.mozilla.org/integration/mozilla-inbound/rev/828358c18fa5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1cc4e505984
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e8619153605
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa3c3232d72
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb7bd25356a7

Comment 53

[Philip Chee]

I'm getting this problem:
c:/t1/hg/comm-central/mozilla/layout/style/nsStyleStruct.cpp(2909) : error C2536: 'nsStyleDisplay::nsStyleDisplay::mTransformOrigin' : cannot specify explicit initializer for arrays
        c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(2578) : see declaration of 'nsStyleDisplay::mTransformOrigin'
c:/t1/hg/comm-central/mozilla/layout/style/nsStyleStruct.cpp(2909) : error C2536: 'nsStyleDisplay::nsStyleDisplay::mPerspectiveOrigin' : cannot specify explicit initializer for arrays
        c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(2580) : see declaration of 'nsStyleDisplay::mPerspectiveOrigin'
c:/t1/hg/comm-central/mozilla/layout/style/nsStyleStruct.cpp(2974) : error C2536: 'nsStyleDisplay::nsStyleDisplay::mTransformOrigin' : cannot specify explicit initializer for arrays
        c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(2578) : see declaration of 'nsStyleDisplay::mTransformOrigin'
c:/t1/hg/comm-central/mozilla/layout/style/nsStyleStruct.cpp(2974) : error C2536: 'nsStyleDisplay::nsStyleDisplay::mPerspectiveOrigin' : cannot specify explicit initializer for arrays
        c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(2580) : see declaration of 'nsStyleDisplay::mPerspectiveOrigin

Comment 54

[Markus Stange [:mstange]]

(In reply to Philip Chee from comment #53)
That sounds like it's from bug 1267524 and not from this bug.

Comment 55

[Daniel Holbert [:dholbert]]

(plus, I think comment 53 may already have been fixed by http://hg.mozilla.org/integration/mozilla-inbound/rev/cf209d9bc29c )

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e83a4f7237e
https://hg.mozilla.org/mozilla-central/rev/828358c18fa5
https://hg.mozilla.org/mozilla-central/rev/f1cc4e505984
https://hg.mozilla.org/mozilla-central/rev/7e8619153605
https://hg.mozilla.org/mozilla-central/rev/dfa3c3232d72
https://hg.mozilla.org/mozilla-central/rev/bb7bd25356a7

Comment 60

[Markus Stange [:mstange]]

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

Comment 61

[Gerry Chang [:gchang]]

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?

Comment 62

[Markus Stange [:mstange]]

Attachment: small backout patch for Aurora 48

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

Comment 64

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8752960 [details] [diff] [review]
small backout patch for Aurora 48

Fix a regression, taking it

Comment 65

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-aurora/rev/9cc06cd6038e

Comment 66

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 48b1, 49.0a2 (2016-06-14) Win 7