If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

initial prototype/impl of BEM styles for small device screens

RESOLVED FIXED

Status

Hello (Loop)
Client
P2
major
Rank:
27
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dmose, Assigned: dmose)

Tracking

unspecified
x86
Mac OS X
Points:
13
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments, 37 obsolete attachments)

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
(Assignee)

Description

3 years ago
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.

Updated

3 years ago
Priority: -- → P2

Updated

3 years ago
Severity: normal → major

Updated

3 years ago
backlog: --- → tech-debt

Updated

3 years ago
Iteration: --- → 38.2 - 9 Feb
(Assignee)

Comment 1

3 years ago
Created attachment 8558709 [details] [diff] [review]
WIP add BEM classes to Loop Markup
(Assignee)

Comment 2

3 years ago
Created attachment 8558731 [details] [diff] [review]
WIP add BEM classes to Loop Markup
Attachment #8558709 - Attachment is obsolete: true
(Assignee)

Comment 3

3 years ago
Created attachment 8559231 [details] [diff] [review]
WIP add BEM classes to Loop Markup
Attachment #8558731 - Attachment is obsolete: true
(Assignee)

Comment 4

3 years ago
Created attachment 8559510 [details] [diff] [review]
WIP add BEM classes to Loop Markup
Attachment #8559231 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Created attachment 8559511 [details] [diff] [review]
WIP add BEMified mobile forks of Loop CSS files
(Assignee)

Comment 6

3 years ago
Created attachment 8559892 [details] [diff] [review]
Add BEM classes to support Hello CSS refactoring
(Assignee)

Comment 7

3 years ago
Created attachment 8559893 [details] [diff] [review]
add forked gecko-touch CSS stylesheets for Hello
(Assignee)

Comment 8

3 years ago
Created attachment 8559894 [details] [diff] [review]
Give loop showcase both default and gecko-touch versions
(Assignee)

Updated

3 years ago
Attachment #8559510 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8559511 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Created attachment 8559897 [details] [diff] [review]
fix SVG icon rendering in Loop showcase
(Assignee)

Comment 10

3 years ago
Created attachment 8559898 [details] [diff] [review]
Switch gecko-touch devices to forked Hello stylesheets
(Assignee)

Comment 11

3 years ago
Created attachment 8560569 [details] [diff] [review]
Add BEM classes to support Hello CSS refactoring
Attachment #8559892 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
Created attachment 8560570 [details] [diff] [review]
add forked gecko-touch CSS stylesheets for Hello
Attachment #8559893 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
Created attachment 8560571 [details] [diff] [review]
Give loop showcase both default and gecko-touch versions
Attachment #8559894 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Created attachment 8560572 [details] [diff] [review]
Switch gecko-touch devices to forked Hello stylesheets
Attachment #8559898 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
Created attachment 8560573 [details] [diff] [review]
Set viewport width=device-width on Loop standalone
(Assignee)

Comment 16

3 years ago
Created attachment 8560575 [details] [diff] [review]
fix SVG icon rendering in Loop showcase
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-
(Assignee)

Comment 18

3 years ago
Created attachment 8562439 [details] [diff] [review]
fix SVG icon rendering in Loop showcase
Attachment #8560575 - Attachment is obsolete: true
Attachment #8562439 - Flags: review?(jaws)
(Assignee)

Comment 19

3 years ago
Created attachment 8562440 [details] [diff] [review]
Add BEM classes to support Hello CSS refactoring
Attachment #8560569 - Attachment is obsolete: true
Attachment #8562440 - Flags: review?(jaws)
(Assignee)

Comment 20

3 years ago
Created attachment 8562441 [details] [diff] [review]
Set viewport width=device-width on Loop standalone
Attachment #8560573 - Attachment is obsolete: true
Attachment #8562441 - Flags: review?(jaws)
(Assignee)

Comment 21

3 years ago
Created attachment 8562443 [details] [diff] [review]
add forked gecko-touch CSS stylesheets for Hello
Attachment #8560570 - Attachment is obsolete: true
Attachment #8562443 - Flags: review?(jaws)
(Assignee)

Comment 22

3 years ago
Created attachment 8562444 [details] [diff] [review]
Give loop showcase both default and gecko-touch versions
Attachment #8560571 - Attachment is obsolete: true
Attachment #8562444 - Flags: review?(jaws)
(Assignee)

Comment 23

3 years ago
Created attachment 8562445 [details] [diff] [review]
Switch gecko-touch devices to forked Hello stylesheets
Attachment #8560572 - Attachment is obsolete: true
Attachment #8562445 - Flags: review?(jaws)
(Assignee)

Comment 24

3 years ago
Patches against fx-team, should apply correctly in order of posting.
(Assignee)

Comment 25

3 years ago
Created attachment 8562910 [details] [diff] [review]
Add BEM classes to support Hello CSS refactoring (apply/review after SVG icon patch)
Attachment #8562440 - Attachment is obsolete: true
Attachment #8562440 - Flags: review?(jaws)
(Assignee)

Comment 26

3 years ago
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)
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 29

3 years ago
(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+
(Assignee)

Updated

3 years ago
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
(Assignee)

Updated

3 years ago
Attachment #8562444 - Attachment is obsolete: false
(Assignee)

Comment 33

3 years ago
(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.
(Assignee)

Comment 35

3 years ago
(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.
(Assignee)

Comment 36

3 years ago
(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.
(Assignee)

Comment 37

3 years ago
(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
(Assignee)

Comment 38

3 years ago
Created attachment 8563814 [details] [diff] [review]
fix SVG icon rendering in Loop showcase
Attachment #8562439 - Attachment is obsolete: true
(Assignee)

Comment 39

3 years ago
Created attachment 8563816 [details] [diff] [review]
Add BEM classes to support Hello CSS refactoring
Attachment #8562910 - Attachment is obsolete: true
Attachment #8563816 - Flags: review+
(Assignee)

Comment 40

3 years ago
Created attachment 8563817 [details] [diff] [review]
Set viewport width=device-width on Loop standalone
Attachment #8562441 - Attachment is obsolete: true
Attachment #8563817 - Flags: review+
(Assignee)

Comment 41

3 years ago
Created attachment 8563818 [details] [diff] [review]
add forked small-screen CSS stylesheets for Hello
Attachment #8562443 - Attachment is obsolete: true
Attachment #8563818 - Flags: review+
(Assignee)

Comment 42

3 years ago
Created attachment 8563821 [details] [diff] [review]
Give loop showcase both default and small-screen versions
Attachment #8562444 - Attachment is obsolete: true
Attachment #8563821 - Flags: review?(jaws)
(Assignee)

Comment 43

3 years ago
Created attachment 8563822 [details] [diff] [review]
Switch small-screen devices to forked Hello stylesheets
Attachment #8562445 - Attachment is obsolete: true
Attachment #8563822 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8563814 - Flags: review+
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 45

3 years ago
(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!
(Assignee)

Comment 46

3 years ago
Created attachment 8564263 [details] [diff] [review]
Give loop showcase both default and small-screen versions
Attachment #8563821 - Attachment is obsolete: true
Attachment #8564263 - Flags: review+
(Assignee)

Comment 47

3 years ago
(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)
(Assignee)

Comment 48

3 years ago
Created attachment 8564776 [details] [diff] [review]
Switch small-screen devices to forked Hello stylesheets
Attachment #8563822 - Attachment is obsolete: true
Attachment #8564776 - Flags: review+
(Assignee)

Comment 49

3 years ago
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.
(Assignee)

Comment 50

3 years ago
Created attachment 8564811 [details] [diff] [review]
Switch small-screen devices to forked Hello stylesheets
Attachment #8564776 - Attachment is obsolete: true
Attachment #8564811 - Flags: review+
(Assignee)

Comment 51

3 years ago
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)
(Assignee)

Comment 53

3 years ago
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
(Assignee)

Comment 54

3 years ago
Created attachment 8565828 [details] [diff] [review]
Add BEM classes to support Hello CSS refactoring
Attachment #8563816 - Attachment is obsolete: true
Attachment #8565828 - Flags: review+
(Assignee)

Comment 55

3 years ago
Created attachment 8565829 [details] [diff] [review]
Set viewport width=device-width on Loop standalone
Attachment #8563817 - Attachment is obsolete: true
Attachment #8565829 - Flags: review+
(Assignee)

Comment 56

3 years ago
Created attachment 8565830 [details] [diff] [review]
add forked small-screen CSS stylesheets for Hello
Attachment #8563818 - Attachment is obsolete: true
Attachment #8565830 - Flags: review+
(Assignee)

Comment 57

3 years ago
Created attachment 8565831 [details] [diff] [review]
Give loop showcase both default and small-screen versions
Attachment #8564263 - Attachment is obsolete: true
Attachment #8565831 - Flags: review+
(Assignee)

Comment 58

3 years ago
Created attachment 8565832 [details] [diff] [review]
Switch small-screen devices to forked Hello stylesheets
Attachment #8564811 - Attachment is obsolete: true
Attachment #8565832 - Flags: review+

Updated

3 years ago
Blocks: 1141493

Updated

3 years ago
Rank: 21
Flags: firefox-backlog+

Comment 59

3 years ago
just putting near other mobile work bug - and moving lower (yet still high) in order to address.
Rank: 21 → 26
(Assignee)

Comment 60

3 years ago
Created attachment 8583315 [details] [diff] [review]
Add BEM classes to support Hello CSS refactoring
Attachment #8565828 - Attachment is obsolete: true
(Assignee)

Comment 61

3 years ago
Created attachment 8583316 [details] [diff] [review]
Set viewport width=device-width on Loop standalone
(Assignee)

Comment 62

3 years ago
Created attachment 8583317 [details] [diff] [review]
add forked small-screen CSS for Loop
Attachment #8565830 - Attachment is obsolete: true
(Assignee)

Comment 63

3 years ago
Created attachment 8583318 [details] [diff] [review]
Give loop showcase both default and small-screen versions
Attachment #8565831 - Attachment is obsolete: true
(Assignee)

Comment 64

3 years ago
Created attachment 8583319 [details] [diff] [review]
Switch small-screen devices to forked Loop stylesheets
Attachment #8565832 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8565829 - Attachment is obsolete: true
(Assignee)

Comment 65

3 years ago
Patches rebased against current fx-team.

Updated

3 years ago
Iteration: 38.2 - 9 Feb → 40.1 - 13 Apr

Updated

3 years ago
Iteration: 40.1 - 13 Apr → ---

Updated

2 years ago
Whiteboard: [tech-debt] → [tech-debt][mobile]

Updated

2 years ago
Rank: 26 → 28

Updated

2 years ago
Rank: 28 → 24

Updated

2 years ago
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
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.