811826 - [Browser] action_menu.css uses invalid "line-height: auto", triggering JavaScript Warning: "Error in parsing value for 'line-height'. Declaration dropped."

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

Noticed this go by in my logcat output:
JavaScript Warning: "Error in parsing value for 'line-height'.  Declaration dropped." {file: "app://browser.gaiamobile.org/style/action_menu.css" line: 84}

Looks like this points to:
 https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/style/action_menu.css#L84
which has:
>   line-height: auto;
which is invalid. line-height doesn't accept any value called "auto". See documentation here:   https://developer.mozilla.org/en-US/docs/CSS/line-height

It does accept "normal" -- that's probably what we want here...?

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer

Hi Daniel - Have a second to review this quick pull request? :)

Comment 2

[Daniel Holbert [:dholbert]]

"normal" is presumably what was intended (instead of "auto"), so this makes sense from that perspective.

However, your patch could conceivably change behavior -- currently, this line is getting completely ignored (since it's invalid), but your patch will make the line actually meaningful.  Given that we're (presumably) doing OK with the invalid CSS, maybe we want to just remove the line?  (Is there any reason we actually need to force "line-height:normal" there? i.e. is there another rule somewhere that imposes a different line-height on an element that this selector matches, and we have to override that?)

Comment 3

[Daniel Holbert [:dholbert]]

FWIW, it looks like the only "line-height" declarations elsewhere in this .css file are setting it to explicit values (1em and 3.8rem) -- we don't bother to explicitly set it to "normal" anywhere else.

(And the selector with "normal" doesn't look like it'd collide with the selectors that set it to 1em / 3.8rem.  So I think it can probably just be dropped...?)

So: I tend to think we should just drop this line, rather than making it "normal" -- at least, that seems at least as reasonable, and it's guaranteed to preserve existing behavior.  I don't really know the context of this CSS, though -- you might want to check with Ben Francis, since git blame says he wrote this code in https://github.com/KevinGrandon/gaia/commit/ae1a43b666cf55405711d9d0c16a65a9f98bba74 , and he can speak more intelligently than I can about the impact or non-impact of your patch.

Comment 4

[Kevin Grandon :kgrandon]

After checking - it appears that this element has no inherited line-height properties and can safely be removed. Pull request has been updated.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 728396 [details]
Github pull request pointer

OK -- happy to r+ the removal, since it's just removing a line that has no effect (and whose intended effect appears to be unnecessary).

Comment 6

[Kevin Grandon :kgrandon]

Landed in master: https://github.com/mozilla-b2g/gaia/commit/15866514ea43151b2b8f87fd92d92a6fd69c17ab