1043318 - [Dialer] [Keypad] On suggestion item when the text goes outside the edge, you can drag the upper part and move the whole screen.

Status: RESOLVED FIXED

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

Description

[Paco Rampas [:paco]]

Attachment: Issue on video

Comment 1

[Paco Rampas [:paco]]

Attachment: SUGGESTION-HORIZONTAL-SCROLL.png

Comment 2

[Paco Rampas [:paco]]

Attachment: patch in github

Comment 3

[Anthony Ricaud (:rik)]

I'm not reproducing this issue. I have a very long contact and it has an ellipsis as expected. Could you give clearer STRs ?

Both si__name and si__details have an ellipsis class [1]. And that class adds an |overflow: hidden|. So I'm surprised to see this.

[1] https://github.com/mozilla-b2g/gaia/blob/fadfafa17f5175203b8b9457bfb95e5816f54f58/apps/communications/dialer/elements/suggestion-item.html#L4-L5
[2] https://github.com/mozilla-b2g/gaia/blob/fadfafa17f5175203b8b9457bfb95e5816f54f58/apps/communications/dialer/style/suggestion.css#L92

Comment 4

[Paco Rampas [:paco]]

1- I have saved a contact with a long name.
2- I have searched the contact on keypad

Result: Appear a suggestion with horizontal scroll.

I have installed a flame build of yesterday. With old builds doesn't happen.
And I have gaia master.

Comment 5

[Paco Rampas [:paco]]

(In reply to Anthony Ricaud (:rik) from comment #3)
> Both si__name and si__details have an ellipsis class [1]. And that class
> adds an |overflow: hidden|. So I'm surprised to see this.


When using flexbox and text-overflow: ellipsis, you need overflow: hidden in the parent of the text you want to truncate.
I have doublechecked that with Arnau.

Comment 6

[Paco Rampas [:paco]]

BTW: The suggestion layer is outside of phone-number-view-container. I think we can improve the code if we include the suggestion item inside it. We can do it here or open a new bug. What do you think?

Comment 7

[Anthony Ricaud (:rik)]

OK, I wasn't seeing because this is a regression of a Gecko change.

Comment 8

[Anthony Ricaud (:rik)]

Daniel: Can you help us verify if it's a Gecko or Gaia bug? I tried reading the spec but it's a collection of links to definitions and couldn't be sure.

We have this:
<div style="flex: 1">
  <div style="overflow: hidden">Very[…]long</div>
  <div style="overflow: hidden">not long</div>
</div>

Because all children are |overflow: hidden|, do we really have to set overflow: hidden or min-width: 0 on the container?

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Anthony Ricaud (:rik) from comment #8)
> Because all children are |overflow: hidden|, do we really have to set
> overflow: hidden or min-width: 0 on the container?

Is the <div style="flex: 1"> overflow:hidden?

That's div is the one whose "overflow" property matters for this min-sizing behavior, because that's the flex item. (I'm assuming its a flex item [child-of-a-flex-container], because it has flex:1 set on it, which only does anything on flex items.)

Its parent (the flex container) will ask for its min-content width, and it returns the min-content width of its widest child, which is the length of Very[...]long).  That ends up being <div style="flex: 1">'s min-width, so it overflows its flex-container-parent.

It sounds to me like the <div style="flex: 1"> wants "min-width: 0".

Comment 10

[Anthony Ricaud (:rik)]

Comment on attachment 8461459 [details] [review]
patch in github

Thanks Daniel. r+ for setting overflow: hidden then.

Comment 11

[Daniel Holbert [:dholbert]]

I think just using "min-width:0" really might be better.  "overflow" is a slightly more heavyweight solution (requires additional frames in the frame tree, to support scrolling), and it's only useful if you're expecting the flex item's contents to overflow outside the flex item's final size *and* you have a way you want to handle that overflow (scrolling, ellipsis, etc).  Neither of those seem to be the case here -- instead, the children themselves are sized to be no wider than the flex item (I think?) and they handle their own overflow with overflow:hidden. So I'm not sure it's useful to add another higher-level layer of "overflow:hidden" here.

Maybe I'm missing something, though.

Comment 12

[Paco Rampas [:paco]]

Merged: ba3654571c09e539a0c8584f29f53438563bb920

Comment 13

[Doug Sherk (:drs) (inactive)]

Paco, can you start putting demos of work like this here?
https://wiki.mozilla.org/FirefoxOS/Comms/Dialer/Sprint/v2.1-S1#Demos

Comment 14

[Daniel Holbert [:dholbert]]

rik or paco, could you look at comment 11, and possibly update the CSS accordingly?

I really think adding "overflow:hidden" on a flex item is not the best workaround for this bug, except in cases where we're explicitly handling overflow *on the flex item*. (and that's not true of this case) It should be more lightweight and performant to use "min-width:0".

Comment 15

[Paco Rampas [:paco]]

Ok Doug, I'm going to put there my demos.

Comment 16

[Doug Sherk (:drs) (inactive)]

(In reply to Paco Rampas [:paco] from comment #15)
> Ok Doug, I'm going to put there my demos.

Thanks!

(In reply to Daniel Holbert [:dholbert] from comment #14)
> rik or paco, could you look at comment 11, and possibly update the CSS
> accordingly?
> 
> I really think adding "overflow:hidden" on a flex item is not the best
> workaround for this bug, except in cases where we're explicitly handling
> overflow *on the flex item*. (and that's not true of this case) It should be
> more lightweight and performant to use "min-width:0".

Do you think we should back this out, or do a followup?

Comment 17

[Paco Rampas [:paco]]

Daniel, very sorry, I have read the comment 11 after merge. I can improve the patch as you said if Antony is agree.

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Doug Sherk (:drs) from comment #16)
> Do you think we should back this out, or do a followup?

Followup is fine, I think. overflow:hidden should still work, & isn't *too* bad; it's just that min-width:0 should be better for this situation.

(In reply to Paco Rampas [:paco] from comment #17)
> Daniel, very sorry, I have read the comment 11 after merge. I can improve
> the patch as you said if Antony is agree.

Thanks! Anthony, does this sound OK to you?

Comment 19

[Anthony Ricaud (:rik)]

Yeah, let's open a follow up bug to deal with this. Can you take care of it Paco? And ask review to Doug.