943183 - [Messages] the bad recipient markers should not be absolutely positioned to a non-scrolled ancestor

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

This is a bug from bug 920546 so asking koi on this one.

I may do a different bug for koi and 1.3, for koi it would be a one-line very safe patch.

I dont have a screenshot right now, will do it soon to help triaging.

Comment 1

[Julien Wajsberg [:julienw]]

STR:
* add enough recipients that the opened recipient panel can scroll
* the last ones should be invalid
* open the recipient panel
* scroll it

Expected:
* the exclamation marks should scroll with the panel

Actual:
* they don't scroll and are even displayed out of their container

The root cause is that they're absolutely positioned but their nearest positioning container is an ancestor of the scrolling panel.

For 1.2, I'll do it as safe as possible adding only a "position: relative" property to the scrolled container.
For 1.3, I'd like to get rid of the absolute positioning that is not necessary here.

see attachment for more information.

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: screenshot showing the issue

Comment 3

[Julien Wajsberg [:julienw]]

Attachment: v1.2 safe patch v1

Github PR is at https://github.com/mozilla-b2g/gaia/pull/14042

* Add "position: relative" for the absolutely positioned marker's nearest
  ancestor.
---
 apps/sms/style/sms.css |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


safest possible patch.

Comment 4

[Julien Wajsberg [:julienw]]

Note: you might need the patch for bug 941353 to review this properly.

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8338254 [details] [diff] [review]
v1.2 safe patch v1

Review of attachment 8338254 [details] [diff] [review]:
-----------------------------------------------------------------

The fixing is good, but I don't understand "get rid of the absolute positioning that is not necessary here". It seems we only set the absolute positioning here. Do we have over absolute positioning element insdie the recipient?

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: master patch v1 (WIP)

A github PR is at https://github.com/mozilla-b2g/gaia/pull/14046

This solution removes all absolute positioning. However, this triggers bug 943228, so we can't apply it until it's resolved or we find a work around.

Therefore, we might want to use the v1.2 patch on master too until bug 943228 is resolved. Steve, what do you think?

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8338305 [details] [diff] [review]
master patch v1 (WIP)

Sounds good that applying v1.2 patch in master temporary because bug 943228 got low priority right now. I'm also good with the patch, maybe we should create another follow-up for the fixing and close this bug with safe one?

Comment 8

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

triage: 1.3+ for broken UI

Comment 9

[Julien Wajsberg [:julienw]]

Preeti, Joe,

The specific patch I did for 1.2 is very safe (one line) so I'd really want to see it landing on 1.2, because when this happens (we need several recipients) this looks very broken. So very small and safe patch for a good value. We can ask Fabrice if you need an additional technical point of view.

Comment 10

[Julien Wajsberg [:julienw]]

Steve: yep, I think it's better to apply the 1.2 fix to master. Since you've already r+ it, I'll merge it tomorrow after I filed a new bug for follow-ups.

Comment 11

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

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Preeti, Joe,
> 
> The specific patch I did for 1.2 is very safe (one line) so I'd really want
> to see it landing on 1.2, because when this happens (we need several
> recipients) this looks very broken. So very small and safe patch for a good
> value. We can ask Fabrice if you need an additional technical point of view.

I think we can ask for approval to land on 1.2. I am fine with it 
ni? Fabrice on technical view

Comment 12

[[:fabrice] Fabrice Desré]

I'm officially only sheriffing 1.3, but that looks good to me for 1.2 also!

Comment 13

[Preeti Raghunath(:Preeti)]

Koi+

Comment 14

[Julien Wajsberg [:julienw]]

Thanks! Now NI John Ford for uplift :)

Comment 15

[John Ford [:jhford] CET/CEST Berlin Time]

Is the master branch patch ready to land?

v1.2: 1e33fd87d61de023b18961270e2f2abe2ead07c1

Comment 16

[Julien Wajsberg [:julienw]]

Nope sorry, I'll land the 1.2 patch on master after discussion with Steve.

And it's not an uplift, I should have landed this myself... Thanks a lot John!

Comment 17

[Julien Wajsberg [:julienw]]

Landed the 1.2 patch on master:
master: 64e8c9c82dbb32624bbeb4d8d1555a311c2e3248

Comment 18

[Julien Wajsberg [:julienw]]

Filed bug 946740 to continue the work about simplifying the CSS for this feature.