821221 - [SMS] Text on "select all" or "deselect all" button disappears when scrolling the "edit" view of a SMS conversation

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P2)

Description

[Walter Chen[:ypwalter][:wachen]]

*Phone: Unagi
2012-12-12
https://releases.mozilla.com/b2g/

*How to reproduce:
  0. send an message to phone A
  1. receive an message from phone A
  2. send an message to phone A
  3. keep sending the messages to make it more than 1.5 pages
  3. click Edit icon on upper-right corner of messages screen
  4. try to scroll up and down

*Expected Result: 
     Nothing happened

*Actual Result:
     The buttons went blank

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

Triage: BB+, C3, P2 - regression for broken feature

Comment 2

[Michał Budzyński (:michalbe)]

According to my research it's not a Gaia bug, nothing changes in the button.

:cjones do you know who can help with this?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Michal Budzynski (:michalbe) from comment #2)
> According to my research it's not a Gaia bug, nothing changes in the button.

If nothing changes after pressing the "Edit" button, that seems like a gaia bug?

> :cjones do you know who can help with this?

The best way is to download nightly builds and find when this behavior regressed.

Comment 4

[Michał Budzyński (:michalbe)]

I think I don't understand. You can get into the EDIT mode, but when you scroll the labels on buttons went blank. From Gaia perspective nothing changes - the label is still set on the button, and CSS classes don't change. Platform is the only explanation here.

Comment 5

[Jet Villegas (inactive)]

To dholbert for a look. Unclear where/when this broke, though bisecting builds may take longer than inspecting the frame tree log.

Comment 6

[Walter Chen[:ypwalter][:wachen]]

I think last time when I report the message can't get selected bug, the buttons are still okay. I believe it's still okay 1~2weeks ago.

Comment 7

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Attachment: frame dump

Well, here's a frame dump from an instance where I reproduced this on trunk.  I think it's a painting/invalidation issue... not sure how do debug those on B2G.  Gonna try for a bit, but I think it may be more efficient to investigate this by doing build bisecting. (regression range tracking)

Comment 8

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

If I add "opacity: 0.99" to .edit-button, here:
   https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L434
it "fixes" this bug (without any side-effects, AFAICT).

So:
 1) That indicates that this is likely from putting things in the wrong part of the display
    list or something... (since setting opacity forces a stacking context, IIRC)
 2) Worst-case, that tweak could be used as a workaround for this.
 3) I wonder if maybe we used to have an opacity specified on these buttons by default -
    if so, that removal might've been what "regressed" this.

Comment 9

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

One other data-point: If I turn disable painting button-backgrounds, then the bug goes away.

In more detail -- if I add an "if(false)" around this chunk (which paints the button's background/border) here...
https://mxr.mozilla.org/mozilla-central/source/layout/forms/nsHTMLButtonControlFrame.cpp?rev=8697feee8fa1&mark=118-121#112
...then the "Select all" text remains visible in cases where this bug would normally disappear.

So I think what's happening here is that we're painting the button background on top of its label, or something like that.

Comment 10

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Also: for the record, I've been unable to reproduce this with the B2G desktop client on my Linux machine -- I can only repro it on actual hardware (my unagi).  If anyone *has* been able to repro it on the desktop, let me know, because it'd be much easier to debug there, if it were possible to reproduce...

Comment 11

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

(at mattwoodrow's suggestion, I checked whether setting gfx.xrender.enabled=false makes this reproducible on B2G desktop client. That didn't seem to have any effect -- still can't repro on desktop.)

Comment 12

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

So -- I did the following:

(a) Toggled gfxUtils::sDumpPaintList to dump display lists
(b) Reproduced the bug (on the "Select all" button), & saved the last display-list
(c) Tapped the "Select all" button (w/ missing text) and then "Deselect all", which returned me to the same selection-state as in (b), except with the button-text now visible, and saved that last display list.

From comparing the display lists, mattwoodrow & I discovered that the display lists at the end of (b) and (c) were **the same**.  (There was only one difference, and it wasn't significant -- we replaced the old opacity-layer around the (disabled) "Deselect all" button with a new one, because it temporarily lost its opacity when it was enabled in the middle of step (c).)

mattwoodrow says this indicates that this is a painting bug.  (since, given the same display list, we're painting different things)  I don't really know how to debug that -- I suspect it's tricky, and mattwoodrow suggests that it might be best to throw a cairo expert at it.  (jrmuizel?)  I don't know how soon that can happen or how long that will take.

In the meantime, I think it's probably best that we just hack around this in the SMS app by adding the one line of CSS that I suggested in comment 8 -- "opacity:0.99" in .edit-button.

Unassigning, so that others can take this and do either or both of those things.  (I might file a pull request for that opacity tweak in the next day or so, if no one else beats me to it.)

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> If I add "opacity: 0.99" to .edit-button, here:
>   
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L434
> it "fixes" this bug (without any side-effects, AFAICT).
> 
> So:
>  1) That indicates that this is likely from putting things in the wrong part
> of the display
>     list or something... (since setting opacity forces a stacking context,
> IIRC)
>  2) Worst-case, that tweak could be used as a workaround for this.
>  3) I wonder if maybe we used to have an opacity specified on these buttons
> by default -
>     if so, that removal might've been what "regressed" this.

Over to Gaia to evaluate the workaround that dholbert proposed in comment 8.

Comment 14

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Attachment: pull request for workaround

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 694485 [details]
pull request for workaround

r+ to land this workaround as long as there is a bug to track the gecko issue. (Could be this one or another)

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/45cb8ba540adbdd24a1b97d26f4bb2bca44c64cc

Since there is a workaround I don't think this bug is still a blocking-basecamp. Renominating just to keep it visible for triagers but should really be minus now.

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) from comment #16)
> https://github.com/mozilla-b2g/gaia/commit/
> 45cb8ba540adbdd24a1b97d26f4bb2bca44c64cc
> 
> Since there is a workaround I don't think this bug is still a
> blocking-basecamp. Renominating just to keep it visible for triagers but
> should really be minus now.

As often Daniel is right and prefer to keep this one blocking+ with a new bug opened to track the platform issue. So let's do that.

Comment 18

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Thanks! I filed bug 824088 on the underlying gecko issue, and I'm resolving this as FIXED.

Comment 19

[Walter Chen[:ypwalter][:wachen]]

Per vivien: "prefer to keep this one blocking+ with a new bug opened to track the platform issue"

Comment 20

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

I think you misunderstood -- vivien wasn't saying this bug here should stay open.  This (blocking) bug (the SMS app being messed up) is now fixed, as of the pull request being merged.

There remains an underlying platform bug which we're now working around, and I spun off bug 824088 for that (which remains open).  But the SMS app should now be working, so this bug here is fixed.

  --> Re-resolving as fixed.

Comment 21

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

(Please do reopen if you can still reproduce with newer versions of gaia than the merge in comment 16, of course)

Comment 22

[Walter Chen[:ypwalter][:wachen]]

Reproduced in 2012-12-23 https://releases.mozilla.com/b2g/.

Comment 23

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Are you sure you had up-to-date Gaia?  (up-to-date releases.mozilla.com/b2g/ wouldn't matter)

Comment 24

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

(I just tried to reproduce for ~5 minutes with an up-to-date just-compiled b2g build on my unagi, and I haven't been able to.)

Comment 25

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Oh, sorry -- I misread that URL as being a version of "git.mozilla.org/releases/b2g" -- didn't realize it had prebuilt nightly snapshots.

I'll try one of those...

Comment 26

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

I downloaded the 12/23 build from that page. Its "More Info" settings screen says
 Git commit info 07:14:27
 2012-12-21 e79b7f4...

I'm not sure what parts (gaia vs b2g vs ??) that datestamp/commit correspond to, but that datestamp is definitely ~5 hours **before** this bug's patch was merged to gaia (in comment 16), so it's entirely possible that that build lacks the fix.

If you're sure it's got the patch (I don't know how to demonstrate conclusively that it does/doesn't), then I'd start to worry...

Comment 27

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

The 12/24 build from https://releases.mozilla.com/b2g/ should definitely have the patch (at least, the "git commit info" has a 12/24 datestamp) -- Walter, would you mind seeing if you can reproduce in that build?

Comment 28

[Walter Chen[:ypwalter][:wachen]]

Verified in 2012/12/24 build

Comment 29

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Phew -- that's good to hear, thanks! :)

Comment 30

[Walter Chen[:ypwalter][:wachen]]

No problem! Thanks for the quick response.