Closed Bug 1119047 Opened 5 years ago Closed 4 years ago

initial prototype/impl of BEM styles for small device screens

Categories

(Hello (Loop) :: Client, defect, P2, major)

x86
macOS
defect
Points:
13

Tracking

(Not tracked)

RESOLVED FIXED
Blocking Flags:
backlog tech-debt

People

(Reporter: dmose, Assigned: dmose)

References

Details

(Whiteboard: [tech-debt][mobile])

Attachments

(5 files, 37 obsolete files)

12.62 KB, patch
Details | Diff | Splinter Review
15.27 KB, patch
Details | Diff | Splinter Review
33.84 KB, patch
Details | Diff | Splinter Review
37.63 KB, patch
Details | Diff | Splinter Review
3.21 KB, patch
Details | Diff | Splinter Review
One thing that contributes to the CSS in Hello being extremely fragile is that the CSS itself is written in a messy hodge-podge of different styles.

On the last day of Mozlandia, we agreed that applying a single coherent style would make it significantly more maintainable.  jaws and others suggested that we use some specific browser files with known reasonable style and simply adapt what we've got to that:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/customizeMode.inc.css

http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css

http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/customizeTip.inc.css

In order tech-debt triage, there was broad agreement that this was a significant pain point and should be a P1, so I suspect it wants to be targeted to 38.
Priority: -- → P2
Severity: normal → major
backlog: --- → tech-debt
Iteration: --- → 38.2 - 9 Feb
Attachment #8558709 - Attachment is obsolete: true
Attachment #8558731 - Attachment is obsolete: true
Attachment #8559231 - Attachment is obsolete: true
Attachment #8559510 - Attachment is obsolete: true
Attachment #8559511 - Attachment is obsolete: true
Attachment #8559892 - Attachment is obsolete: true
Attachment #8559893 - Attachment is obsolete: true
Attachment #8559898 - Attachment is obsolete: true
Attachment #8559897 - Attachment is obsolete: true
Comment on attachment 8560572 [details] [diff] [review]
Switch gecko-touch devices to forked Hello stylesheets

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

::: browser/components/loop/standalone/content/index.html
@@ +13,5 @@
> +
> +    <!-- used on desktop browsers, and non-Gecko mobile browsers (eg mobile
> +         chrome)-->
> +    <link rel="stylesheet" type="text/css" href="shared/css/conversation.css"
> +          media="not screen and (-moz-touch-enabled)">

These styles will not apply on touch-screen ultrabooks (like my Lenovo x1 carbon).
Attachment #8560572 - Flags: review-
Attachment #8560575 - Attachment is obsolete: true
Attachment #8562439 - Flags: review?(jaws)
Attachment #8560569 - Attachment is obsolete: true
Attachment #8562440 - Flags: review?(jaws)
Attachment #8560573 - Attachment is obsolete: true
Attachment #8562441 - Flags: review?(jaws)
Attachment #8560570 - Attachment is obsolete: true
Attachment #8562443 - Flags: review?(jaws)
Attachment #8560571 - Attachment is obsolete: true
Attachment #8562444 - Flags: review?(jaws)
Attachment #8560572 - Attachment is obsolete: true
Attachment #8562445 - Flags: review?(jaws)
Patches against fx-team, should apply correctly in order of posting.
Comment on attachment 8562910 [details] [diff] [review]
Add BEM classes to support Hello CSS refactoring (apply/review after SVG icon patch)

Rebased this patch against latest fx-team.  Note that this means that the patches above are no longer quite in order (as indicated by the title of this one).
Attachment #8562910 - Flags: review?(jaws)
Attachment #8562910 - Attachment description: Add BEM classes to support Hello CSS refactoring (apply after SVG icon patch) → Add BEM classes to support Hello CSS refactoring (apply/review after SVG icon patch)
Attachment #8562439 - Flags: review?(jaws) → review+
Attachment #8562441 - Flags: review?(jaws) → review+
Comment on attachment 8562444 [details] [diff] [review]
Give loop showcase both default and gecko-touch versions

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

::: browser/components/loop/ui/ui-showcase.jsx
@@ +9,5 @@
>  
> +/*
> + * XXX The small-screen version of this is a work in progress.  As of this
> + * writing, it merely unloads the old stylesheets, and loads the small-screen
> + * style sheets and then hides the desktop-only components.  Two big things

Instead of unloading stylesheets, can we do this with alternate stylesheets? http://www.w3.org/Style/Examples/007/alternatives.en.html
Attachment #8562444 - Flags: review?(jaws)
Comment on attachment 8562445 [details] [diff] [review]
Switch gecko-touch devices to forked Hello stylesheets

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

r+ with the following two tasks complete.

::: browser/components/loop/standalone/content/index.html
@@ +13,5 @@
>      <link rel="stylesheet" type="text/css" href="shared/css/reset.css">
> +
> +    <!-- Because we use min- and max-device width in these queries, they
> +         should effectively never load on desktop views, because no
> +         desktop screens these days have widths less 599px.

Can you please file a follow-up bug for switching this from using device-width to max-width? We should show a reasonable UI on desktop for people who don't use a maximized window for the standalone client.

@@ +28,5 @@
> +    <link rel="stylesheet" type="text/css" href="css/webapp.css"
> +          media="(min-device-width: 481px) and (orientation: portrait)">
> +
> +    <link rel="stylesheet" type="text/css" href="shared/css/conversation.css"
> +          media="(min-device-width: 599px) and (orientation: landscape)">

These don't need to be duplicated. Media queries accept a comma-separated list which acts as a logical-or. 
https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Media_queries#comma-separated_lists
Attachment #8562445 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #27)
>
> Instead of unloading stylesheets, can we do this with alternate stylesheets?
> http://www.w3.org/Style/Examples/007/alternatives.en.html

Interesting idea re alternate stylesheets.  Without using similar JS to force them to be non-alternate given the appropriate parameter, it seems like the dev experience is a bit worse, since the developer needs to go to "view -> style -> style name", for which there is no keyboard shortcut.  Which makes me expect that they'd do so less.

Since we'd still need to do similar JS frobbing, the extra work to make that change doesn't really feel like a win to me.
Comment on attachment 8562910 [details] [diff] [review]
Add BEM classes to support Hello CSS refactoring (apply/review after SVG icon patch)

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

::: browser/components/loop/content/shared/js/views.jsx
@@ +62,4 @@
>        return cx(classesObj);
>      },
>  
> +    _upCaseFirst: function(str) {

s/_upCaseFirst/_sentenceCase/
Attachment #8562910 - Flags: review?(jaws) → review+
Attachment #8562444 - Attachment is obsolete: true
Comment on attachment 8562443 [details] [diff] [review]
add forked gecko-touch CSS stylesheets for Hello

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

Grudgingly r+, much of the CSS is still filled with cross-browser hacks and specializations that would be nice if we could get rid of.

I'm not sure how many are really necessary. For example, I don't know what we float, but we're including a clearfix hack that mentions it is needed if a contenteditable is in the page (but we don't use any contenteditable elements).

::: browser/components/loop/content/shared/css/common-small-screen.css
@@ +5,5 @@
> +/* Generic rules */
> +
> +/**
> + * "Fixes" the Box Model.
> + * @see http://www.paulirish.com/2012/box-sizing-border-box-ftw/

I know you're just copying this, but it would be nice to see this go away. I filed bug 1132615 to get rid of this.

@@ +398,5 @@
> +}
> +
> +/* Logos */
> +
> +.firefox-logo {

Can you please file a bug to change this to s/firefox-logo/brand-logo/ ?

@@ +402,5 @@
> +.firefox-logo {
> +  margin: 0 auto; /* horizontal block centering */
> +  width: 100px;
> +  height: 100px;
> +  background: transparent url(../img/firefox-logo.png) no-repeat center center;

Can you please file a bug about changing this to reference a branding directory?

::: browser/components/loop/content/shared/css/conversation-small-screen.css
@@ +51,5 @@
> +}
> +
> +.ConversationToolbar .btn {
> +  /* dimensions according to spec
> +   * https://people.mozilla.org/~dhenein/labs/loop-link-spec/#video-call */

This is not a permanent URL. You change this link to reference something on Bugzilla, or we could remove this comment altogether since arguably all styling is done according to the spec.

@@ +111,5 @@
> +/*************/
> +
> +.standalone .MediaLayout-LocalSDKContainer {
> +  /* required to have it superimposed to the control toolbar */
> +  z-index: 1001;

Some places z-index:999 is used and here z-index:1001 is used. Should we be leaving more space between these items?

@@ +574,5 @@
> +.room-invitation-overlay textarea {
> +  display: block;
> +  background: rgba(0, 0, 0, .5);
> +  color: #fff;
> +  font-family: "Helvetica Neue", Arial, sans;

I don't think Helvetica Neue is available on Android (Open Sans instead?). Also, I think you want `sans-serif` here instead of `sans`.

::: browser/components/loop/standalone/content/css/webapp-small-screen.css
@@ +14,5 @@
> +  width: 100%;
> +  background: #fbfbfb;
> +  color: #666;
> +  text-align: center;
> +  font-family: Open Sans,sans-serif;

nit, space after the comma.

@@ +214,5 @@
> +}
> +
> +.btn-large {
> +  /* Dimensions from spec
> +   * https://people.mozilla.org/~dhenein/labs/loop-link-spec/#call-start */

Ditto wrt to the URL.
Attachment #8562443 - Flags: review?(jaws) → review+
(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #29)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #27)
> >
> > Instead of unloading stylesheets, can we do this with alternate stylesheets?
> > http://www.w3.org/Style/Examples/007/alternatives.en.html
> 
> it seems like the dev experience is a bit worse, since the developer needs to go to
> "view -> style -> style name", for which there is no keyboard shortcut. 

On Windows it is Alt+V, Y, then the name of the alternate stylesheet. It is true that Mac doesn't provide good keyboard accessibility, but I don't see that as a strong enough reason to not use a standard way for switching stylesheets.

> Since we'd still need to do similar JS frobbing, the extra work to make that
> change doesn't really feel like a win to me.

The JS is much more straightforward for switching to an alternate stylesheet than it is to unload them from the DOM and insert new stylesheet elements.
Status: NEW → ASSIGNED
Attachment #8562444 - Attachment is obsolete: false
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #31)
> Comment on attachment 8562443 [details] [diff] [review]
> add forked gecko-touch CSS stylesheets for Hello
> 
> Review of attachment 8562443 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Grudgingly r+, much of the CSS is still filled with cross-browser hacks and
> specializations that would be nice if we could get rid of.

Agree that we want to get rid of this stuff, and soon.  For now, I've been attacking the cleanup incrementally.  After we get a bit further along and decide on our medium-term strategy here, I think that will inform what our short-term tactics are.  I suspect running the devtools CSS coverage tool will end up being one of this.

> I'm not sure how many are really necessary. For example, I don't know what
> we float, but we're including a clearfix hack that mentions it is needed if
> a contenteditable is in the page (but we don't use any contenteditable
> elements).

In this case, it was easy to convince myself by grepping that the .cf class is currently unused, so I've killed that in the fork now.
 
> ::: browser/components/loop/content/shared/css/common-small-screen.css
> @@ +5,5 @@
> > +/* Generic rules */
> > +
> > +/**
> > + * "Fixes" the Box Model.
> > + * @see http://www.paulirish.com/2012/box-sizing-border-box-ftw/
> 
> I know you're just copying this, but it would be nice to see this go away. I
> filed bug 1132615 to get rid of this.

Thanks for filing!
 
> @@ +398,5 @@
> > +}
> > +
> > +/* Logos */
> > +
> > +.firefox-logo {
> 
> Can you please file a bug to change this to s/firefox-logo/brand-logo/ ?

Bug 1132725 on file.

> @@ +402,5 @@
> > +.firefox-logo {
> > +  margin: 0 auto; /* horizontal block centering */
> > +  width: 100px;
> > +  height: 100px;
> > +  background: transparent url(../img/firefox-logo.png) no-repeat center center;
> 
> Can you please file a bug about changing this to reference a branding
> directory?

Bug 1132727 on file.

> @@ +111,5 @@
> > +/*************/
> > +
> > +.standalone .MediaLayout-LocalSDKContainer {
> > +  /* required to have it superimposed to the control toolbar */
> > +  z-index: 1001;
> 
> Some places z-index:999 is used and here z-index:1001 is used. Should we be
> leaving more space between these items?

Probably, yes.  I haven't changed any z-index stuff in this patch, and I want to keep that out of scope for the BEM refactoring.  I'm trying very hard to avoid biting off more than we can chew by confining this refactoring to things that are causing us significant amounts of pain (specifically selector naming, and overly complex rule-sharing). 

> @@ +574,5 @@
> > +.room-invitation-overlay textarea {
> > +  display: block;
> > +  background: rgba(0, 0, 0, .5);
> > +  color: #fff;
> > +  font-family: "Helvetica Neue", Arial, sans;
> 
> I don't think Helvetica Neue is available on Android (Open Sans instead?).
> Also, I think you want `sans-serif` here instead of `sans`.

Excitingly, it turns out that .room-invitation-overlay isn't used in the standalone client at all, so I've just killed those rules.

All the other comments, I've just fixed.
(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #33)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #31)
> > @@ +574,5 @@
> > > +.room-invitation-overlay textarea {
> > > +  display: block;
> > > +  background: rgba(0, 0, 0, .5);
> > > +  color: #fff;
> > > +  font-family: "Helvetica Neue", Arial, sans;
> > 
> > I don't think Helvetica Neue is available on Android (Open Sans instead?).
> > Also, I think you want `sans-serif` here instead of `sans`.
> 
> Excitingly, it turns out that .room-invitation-overlay isn't used in the
> standalone client at all, so I've just killed those rules.

Ok, well, then we'll want to make those same changes in the files that they were forked from. http://mxr.mozilla.org/mozilla-central/search?string=sans%3B&find=loop&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central for example. I'm OK with you deleting those lines in the origin files in this patch.

Also, I hate to do this to you, but it would be better if these files were `hg copy`d from the original files so that `hg blame` will transfer over and you won't get blamed for all of the CSS in these files.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32)
> > Since we'd still need to do similar JS frobbing, the extra work to make that
> > change doesn't really feel like a win to me.
> 
> The JS is much more straightforward for switching to an alternate stylesheet
> than it is to unload them from the DOM and insert new stylesheet elements.

Fixed in the next version of that patch, which I'll put up for review shortly.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #28)

> Can you please file a follow-up bug for switching this from using
> device-width to max-width? We should show a reasonable UI on desktop for
> people who don't use a maximized window for the standalone client.

Bug 1132766 filed.

> @@ +28,5 @@
> > +    <link rel="stylesheet" type="text/css" href="css/webapp.css"
> > +          media="(min-device-width: 481px) and (orientation: portrait)">
> > +
> > +    <link rel="stylesheet" type="text/css" href="shared/css/conversation.css"
> > +          media="(min-device-width: 599px) and (orientation: landscape)">
> 
> These don't need to be duplicated. Media queries accept a comma-separated
> list which acts as a logical-or.

Ah, nice, thanks for turning me on to that.  Switched over.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #34)
>
> Ok, well, then we'll want to make those same changes in the files that they
> were forked from.
> http://mxr.mozilla.org/mozilla-central/
> search?string=sans%3B&find=loop&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozil
> la-central for example. I'm OK with you deleting those lines in the origin
> files in this patch.
> 
> Also, I hate to do this to you, but it would be better if these files were
> `hg copy`d from the original files so that `hg blame` will transfer over and
> you won't get blamed for all of the CSS in these files.

As per IRC discussion, I'll use "hg copy" to generate the original versions of the files that actually land, but I won't try to backport all the changes, in the interest of limiting scope and moving fast.
Blocks: 1132766
Attachment #8562439 - Attachment is obsolete: true
Attachment #8562910 - Attachment is obsolete: true
Attachment #8563816 - Flags: review+
Attachment #8562441 - Attachment is obsolete: true
Attachment #8563817 - Flags: review+
Attachment #8562443 - Attachment is obsolete: true
Attachment #8563818 - Flags: review+
Attachment #8562444 - Attachment is obsolete: true
Attachment #8563821 - Flags: review?(jaws)
Attachment #8562445 - Attachment is obsolete: true
Attachment #8563822 - Flags: review+
Attachment #8563814 - Flags: review+
Summary: apply one coherent CSS style to the loop CSS → initial prototype/impl of BEM styles for small device screens
Comment on attachment 8563821 [details] [diff] [review]
Give loop showcase both default and small-screen versions

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

::: browser/components/loop/ui/ui-showcase.jsx
@@ +8,5 @@
>  /* global loop:true, React */
>  
> +/*
> + * XXX The small-screen version of this is a work in progress.  As of this
> + * writing, it merely unloads the old stylesheets, and loads the small-screen

Now that we're switching to alternate stylesheets, we can change this comment to:
"it switches to a small-screen alternate stylesheet and disables the default stylesheet, hiding the desktop-only components.

@@ +227,5 @@
> +      var links = document.getElementsByTagName('link');
> +      for (var i = links.length - 1; i >= 0; i--) {
> +
> +        /* if this is a stylesheet with a title attribute... */
> +        if (links[i].getAttribute('rel').indexOf('style') > -1 &&

Please use double-quotes around the strings. We can remove this comment and the one below, as the code already describes what it is doing here pretty clearly.

@@ +232,5 @@
> +            links[i].getAttribute('title')) {
> +
> +          /* turn it on if it has the specified title; off otherwise */
> +          if (links[i].getAttribute('title') == title) {
> +            links[i].disabled = false;

This can be simplified to:
links[i].disabled = links[i].getAttribute("title") != title;

(I wish we could use `for ... of` loops here, as then we could get rid of the [i] crud, but we need to keep this for cross-browser compat.)
Attachment #8563821 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #44)

> Now that we're switching to alternate stylesheets, we can change this
> comment to:
> "it switches to a small-screen alternate stylesheet and disables the default
> stylesheet, hiding the desktop-only components.

Good catch; fixed.

> Please use double-quotes around the strings. We can remove this comment and
> the one below, as the code already describes what it is doing here pretty
> clearly.

Fixed.

> @@ +232,5 @@
> > +            links[i].getAttribute('title')) {
> > +
> > +          /* turn it on if it has the specified title; off otherwise */
> > +          if (links[i].getAttribute('title') == title) {
> > +            links[i].disabled = false;
> 
> This can be simplified to:
> links[i].disabled = links[i].getAttribute("title") != title;

While this is terser, and in a mathematical sense more elegant, I find it harder/slower to read.  In the interest of maintainability, I'm going to leave it in the more skimmable form.  I'm happy to chat about this more for a future patch, if you feel strongly about it. 
 
> (I wish we could use `for ... of` loops here, as then we could get rid of
> the [i] crud, but we need to keep this for cross-browser compat.)

Totally.  Won't be _too_ far in the future, happily!
Attachment #8563821 - Attachment is obsolete: true
Attachment #8564263 - Flags: review+
(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #37)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #34)
>
> > Also, I hate to do this to you, but it would be better if these files were
> > `hg copy`d from the original files so that `hg blame` will transfer over and
> > you won't get blamed for all of the CSS in these files.
> 
> As per IRC discussion, I'll use "hg copy" to generate the original versions
> of the files that actually land, but I won't try to backport all the
> changes, in the interest of limiting scope and moving fast.

After reviewing the "Copying files" section of <http://hgbook.red-bean.com/read/mercurial-in-daily-use.html>, this looks like serious footgun to me.  Our CSS is enough of a mess that having changes auto-propagate across the first merge just seems like it's asking for trouble.

Given all the upcoming changes I've already got written and queued up (removing .standlone, removing a bunch of unnecessary media queries, moving lots of hunks around in the conversation-small-screen.css files so that related rulesets are in the same place, I think that even if I were to do the copy, the original blame would quickly become largely obscured behind my commits.  That said, this if we _really_ think the original blame is likely to be useful (which I expect to be relatively rare, given how much this CSS is and will change in the coming weeks), we can always go look at the blame on the pre-forked files.

I'd strongly recommend that we land this without doing the hg copy. Ok?
Flags: needinfo?(jaws)
Attachment #8563822 - Attachment is obsolete: true
Attachment #8564776 - Flags: review+
This latest iteration of the patch addresses a problem where I had only taken jaws' review request to coalesce media queries on the small screen queries.  Now both small and large screen use compound queries.  Carrying forward r+, as this is a minor change.
Attachment #8564776 - Attachment is obsolete: true
Attachment #8564811 - Flags: review+
Latest attachment just cleans up the size of devices where we use the small stylesheets to be consistent with rest of the code as well as to work on both Firefox and Chrome.  Carrying forward r+.
(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #47)
> After reviewing the "Copying files" section of
> <http://hgbook.red-bean.com/read/mercurial-in-daily-use.html>, this looks
> like serious footgun to me.

I don't see where in that page it says it is a foot-gun. We've made copies of many files this way, and even though we may be planning large-scale changes that may obfuscate most of the file, it will still be very helpful down the road.

Doing this through hg copy should be a simple task if you (1) make a local copy of your changes, (2) delete the new file, (3) use hg copy to make a copy, and then (4) do a diff between your local copy and the new hg copy and that will show you what you need to change in the hg-copy'd version.
Flags: needinfo?(jaws)
Comment on attachment 8563814 [details] [diff] [review]
fix SVG icon rendering in Loop showcase

Obsoleted by other changes on fx-team.
Attachment #8563814 - Attachment is obsolete: true
Attachment #8563816 - Attachment is obsolete: true
Attachment #8565828 - Flags: review+
Attachment #8563817 - Attachment is obsolete: true
Attachment #8565829 - Flags: review+
Attachment #8563818 - Attachment is obsolete: true
Attachment #8565830 - Flags: review+
Attachment #8564263 - Attachment is obsolete: true
Attachment #8565831 - Flags: review+
Attachment #8564811 - Attachment is obsolete: true
Attachment #8565832 - Flags: review+
Blocks: 1141493
Rank: 21
Flags: firefox-backlog+
just putting near other mobile work bug - and moving lower (yet still high) in order to address.
Rank: 21 → 26
Attachment #8565830 - Attachment is obsolete: true
Attachment #8565829 - Attachment is obsolete: true
Patches rebased against current fx-team.
Iteration: 38.2 - 9 Feb → 40.1 - 13 Apr
Iteration: 40.1 - 13 Apr → ---
Whiteboard: [tech-debt] → [tech-debt][mobile]
Rank: 26 → 28
Rank: 28 → 24
Rank: 24 → 27
The prototype was deemed insufficiently useful to continue with.  Probably CSS Modules would be a better approach if we try this in the future.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.