1050431 - Fix or drop CSS in Gaia video app that uses old "auto" keyword for flex-basis

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

As discussed in bug 1032922, the flex-basis keyword "auto" is being renamed to "main-size". I'm implementing that over in that bug.

So, to avoid affecting gaia behavior, we need to replace all "flex-basis:auto" with "flex-basis:main-size"

This affects the "flex" shorthand, as well, since it sets "flex-basis". We need to replace e.g. "flex: 1 1 auto" with "flex: 1 1 main-size", and similar. 

(Though "flex:auto" is special case & doesn't need to change; that's now a special keyword for the "flex" shorthand, which expands to "1 1 main-size".)

Comment 1

[Daniel Holbert [:dholbert]]

There aren't actually many things that need fixing, as it turns out.

I ran this in my gaia clone:
> grep -r "flex.*auto" *
and I came up with 5 instances of "flex: auto" (which don't need fixing), and 3 instances of "flex: 0 1 auto" in video.css that do need fixing.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: pull request

Here's the patch.

Note that this should *not* be merged until after bug 1032922 lands (to create the "main-size" keyword in Gecko).

Comment 3

[Daniel Holbert [:dholbert]]

(TANGENTIAL NOTE: this change technically may not be required for several reasons:
 (1) The 'main-size' keyword just retrieves the value of [width|height] -- so if that property has the value "auto" (which is likely), then flex-basis:main-size & flex-basis:auto are basically equivalent.
 (2) The style used here, "flex: 0 1 auto" (becoming "flex: 0 1 main-size" after the rename) is equivalent to the initial values of these properties. So it's really equivalent to just using "flex: initial"; and in fact, these lines might be able to be removed entirely, unless we have some conflicting "flex" properties set somewhere else that we're trying to override here.)

Anyway, I'm not going to worry about these tangential notes too much -- applying the rename is IMHO the safe thing to do, since it will preserve existing behavior.  Subsequent cleanup can happen elsewhere, if it's worth doing.

Comment 4

[David Flanagan [:djf]]

Comment on attachment 8469647 [details] [review]
pull request

I obviously have not tested this, but the change seems fine to me.

If, as you note in comment 3, hwoever, the flex: declarations just specify the default values and don't do anything, I'd rather just remove them.

Setting needinfo for John, who introduced this code. John: do you think we could just remove the flex properties entirely?

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

I am afraid that we may not to remove them. I use it to meet the visual spec of Tablet bug 903920. That's to put title, duration, and size/type text align to bottom-right corner. And when title becomes longer, the text still sit at bottom-right corner but taller.

If we want to remove the code, we may need to use JavaScript to calculate the position dynamically. Or we may just remove them and drop the visual requirement of tablet since it may be changed in the future.

IMHO, I would suggest to |remove them| because the tablet is a mass currently. But we should still keep phone work as normal while removing them.

Comment 6

[Daniel Holbert [:dholbert]]

I'm not quite sure I understand what you're saying, but let me clarify what I'm saying & why I suggested removing them:

The value we're using here, "flex: 0 1 auto" (now being renamed to "flex: 0 1 main-size"), is just setting the initial (default) value of these properties.

So, it literally should have no effect. If it has an effect, there's something interesting/strange going on.

Comment 7

[Daniel Holbert [:dholbert]]

(The only way it would have an effect would be if the Video app had another style rule explicitly setting these properties to something else (to a non-default value) on these elements. I don't think that's the case here, but if it is, it'd make more sense why we'd need this here.)

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

Daniel,

Thanks for your information. You are correct. We should remove it.

I had tried to remove them and retest them. It looks the same. I cannot remember correctly why I put "fix: 0 1 auto". But the comment 5 is stating the "display: flex" and other "flex" related css settings.

Comment 9

[Daniel Holbert [:dholbert]]

OK; tomorrow I'll post a new pull request to just remove these lines, then. Thanks!

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: updated pull request (just dropping the decls)

Comment 11

[David Flanagan [:djf]]

Comment on attachment 8472661 [details] [review]
updated pull request (just dropping the decls)

The patch looks good to me. I haven't tested it personally, but it sounds like John did

Comment 12

[Daniel Holbert [:dholbert]]

Thanks! Adding checkin-needed to get this merged.

(I'm confident enough that these lines have no effect that I'm not concerned about this needing any serious testing. I'm also no longer worried about the scenario in comment 7 being a risk -- I looked at the original commit[1] that added this "flex" styling, and these were the only usages of "flex" & its sub-properties (flex-grow/shrink/basis). So, these lines aren't there to override other CSS, which means they indeed have no effect since they're just setting the initial values.)

[1] https://github.com/mozilla-b2g/gaia/commit/7845443b26eb036691e92d1296cdf0fae5dccc12

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/fd4979d71071607bb3192da31233aac96fe86353