Closed Bug 943183 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

RESOLVED FIXED
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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.
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.
Attachment #8338254 - Flags: review?(schung)
Note: you might need the patch for bug 941353 to review this properly.
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?
Attachment #8338254 - Flags: review?(schung) → review+
Attached patch master patch v1 (WIP) (obsolete) — Splinter Review
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?
Attachment #8338305 - Flags: feedback?(schung)
blocking-b2g: koi? → 1.3?
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?
Attachment #8338305 - Flags: feedback?(schung) → feedback+
triage: 1.3+ for broken UI
blocking-b2g: 1.3? → 1.3+
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.
Flags: needinfo?(praghunath)
Flags: needinfo?(jcheng)
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.
(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
Flags: needinfo?(jcheng) → needinfo?(fabrice)
I'm officially only sheriffing 1.3, but that looks good to me for 1.2 also!
Flags: needinfo?(fabrice)
blocking-b2g: 1.3+ → koi?
Flags: needinfo?(jcheng)
Koi+
blocking-b2g: koi? → koi+
Flags: needinfo?(praghunath)
Flags: needinfo?(jcheng)
Thanks! Now NI John Ford for uplift :)
Flags: needinfo?(jhford)
Keywords: checkin-needed
Is the master branch patch ready to land?

v1.2: 1e33fd87d61de023b18961270e2f2abe2ead07c1
Flags: needinfo?(jhford)
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!
Keywords: checkin-needed
Landed the 1.2 patch on master:
master: 64e8c9c82dbb32624bbeb4d8d1555a311c2e3248
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8338305 - Attachment is obsolete: true
Filed bug 946740 to continue the work about simplifying the CSS for this feature.
You need to log in before you can comment on or make changes to this bug.