1861248 - Fix ASRouter devtools styles

Status: RESOLVED FIXED

(Firefox :: Messaging System, defect, P3)

Description

[Shane Hughes [:aminomancer]]

Some CSS issues with the devtools:

• The content area is too wide so that it overflows needlessly.
• There's no space between the right edge of the content area and the page.
• The wrench icon that opens the devtools has a pointer cursor but the gear icon doesn't.
• The wrench icon's position is different when the devtools are open than when the devtools are closed.
• The wrench icon's border radius is not the same as the gear icon's.
• The wrench icon's .expanded background is dark, but the icons' hover and active backgrounds are light.
• The wrench icon's .expanded styles override its hover and active styles, so there's no visible feedback indicating that you can close the devtools by clicking the wrench icon.

Comment 1

[Shane Hughes [:aminomancer]]

I think the tables should be converted into flexboxes to give us more control over flow and to change the message ID from a cell with its own column into a row above the message's buttons and textbox. There's a ton of wasted space due to message IDs being shown in a column with mostly empty space in each message ID cell.

Comment 2

[Shane Hughes [:aminomancer]]

An additional visual bug is mentioned in bug 1862354. We can fix that here.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:lsmith, could you have a look please?

For more information, please visit BugBot documentation.

Comment 4

[Shane Hughes [:aminomancer]]

Also add common.css and switch to the typography variables (like --font-weight-bold) for font styles.

Comment 5

[Shane Hughes [:aminomancer]]

Attachment: Bug 1861248 - Overhaul ASRouter admin and remove its dependency on newtab. r=#omc-reviewers

For starters, replace all newtab styles and functions. Since we're
removing these styles, I conformed the asrouter admin to the reusable
components team's design tokens. So, it now uses global system page
styles. This is a pretty general overhaul since there are so many styles
to replace. In addition to the style changes, I've added a new Filters
UI and moved the groups table to the General tab. This allows us to
remove the Message Groups tab since that functionality is now rolled
into the Filters UI. The same with the Private Browsing tab: when you
hit Show on a pb_newtab message, it will open a PB window and override
the message. And you can filter by template now, so you can view only PB
messages on the General tab. I also fixed spellchecking. Instead of
spellchecking, which only works for natural languages, we just validate
that the text is valid JSON. If it's not valid, we show a red border on
the textarea. That way messages won't just mysteriously fail to show. I
also moved a few elements around to conserve space. Now, everything
should be able to fit on the screen of a default window size of 1500px.
Finally, I removed some old cruft that was left over from when the admin
interface was part of the newtab page.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a766187f9b5
Overhaul ASRouter admin and remove its dependency on newtab. r=omc-reviewers,emcminn

Comment 7

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/3a766187f9b5

Comment 8

[Punam Dahiya [:pdahiya]]

Noticed patch removed message group and groups are showing under general along with rest of the messages. Is that intended? Filtering by provider cfr shows all messages under CFR provider blocked with message group blocked and no easy way to come out? Click of unblock respective to message doesn't seem to work ( please see attached).
Steps to replicate

1. Open about:asrouter#devtools
2. Filter by groups , check cfr
3. Shows all message blocked

Comment 9

[Punam Dahiya [:pdahiya]]

Attachment: ASRouter Admin blocked messages.png

Comment 10

[Shane Hughes [:aminomancer]]

Attachment: pre-patch behavior

(In reply to Punam Dahiya [:pdahiya] from comment #8)

Noticed patch removed message group and groups are showing under general along with rest of the messages. Is that intended?

Yes, since I added a group filter it made the Message Groups tab obsolete, so I put the groups table in the General tab under the providers table.

Filtering by provider cfr shows all messages under CFR provider blocked with message group blocked and no easy way to come out? Click of unblock respective to message doesn't seem to work ( please see attached).
Steps to replicate

1. Open about:asrouter#devtools
2. Filter by groups , check cfr
3. Shows all message blocked

It looks like you have browser.newtabpage.activity-stream.asrouter.userprefs.cfr.features set to false, so all groups that specify that for userPreferences are disabled (including cfr). The patch hasn't changed anything there, see attached image for the pre-patch behavior. So this looks expected to me, unless I'm misunderstanding the issue.

Comment 11

[Shane Hughes [:aminomancer]]

You have given me an idea though - we could improve the UI by not showing the "Unblock" button if a message is not individually blocked. The button is confusing UI since it implies the message is blocked, when in reality it's the group that's blocked (or the provider is excluding the message, though we don't currently have any provider exclusions). So clicking the button will have no real effect.

I guess we could potentially show an "Unblock group" button or "Remove provider exclusion" button, when those functions would be relevant. It's easy to remove an exclusion since those are just part of the JSON value of the provider pref. But groups are tricky since they can be disabled as a result of prefs or by simply setting enabled: false in the Remote Settings record. If it's just disabled by pref, then it's possible to re-enable by setting all the prefs to true. But if it's disabled in the base JSON definition, then there's nothing Firefox can do to re-enable the group on RS.

The trouble is that ASRouter overwrites group.enabled with the userPreferences values. I suppose we could add a new property group.userEnabled and leave the original group.enabled alone. Then we'd be able to check whether the group is fully disabled on RS, or just blocked by pref. So if group.enabled && !group.userEnabled, we could show an "Unblock group" button that would re-enable the group's prefs. But if !group.enabled then we'd just show no button at all, since there's no way to unblock the group.

Another nice feature would be to add a way to toggle group preferences, when applicable. The user preferences column in the groups table could have checkboxes added for each pref, so you could easily toggle them without needing to go to about:config and search the pref.

Perhaps when a message is showing as blocked because of message group blocked, and it's because of a user preference, we could show the name of that preference and put a checkbox toggle there as well. That might be better than an "unblock group" button since it's more transparent what's going on.

Edit: It turns out provider exclusions are irrelevant anyway because we literally filter excluded messages out of the messages array. So excluded messages aren't given to ASRouterAdmin in the first place. Rather than showing as blocked, they just aren't rendered at all.

Comment 12

[Punam Dahiya [:pdahiya]]

(In reply to Shane Hughes [:aminomancer] from comment #10)

Created attachment 9409064 [details]
pre-patch behavior

(In reply to Punam Dahiya [:pdahiya] from comment #8)

Noticed patch removed message group and groups are showing under general along with rest of the messages. Is that intended?

Yes, since I added a group filter it made the Message Groups tab obsolete, so I put the groups table in the General tab under the providers table.

My take by combining Messages and Message groups in a single page has added a lot of info to process pushing down the actual functionality specific to page below the fold.

General -> Showing all message JSONs , Message Group -> All messages in a group

Filtering by provider cfr shows all messages under CFR provider blocked with message group blocked and no easy way to come out? Click of unblock respective to message doesn't seem to work ( please see attached).
Steps to replicate

1. Open about:asrouter#devtools
2. Filter by groups , check cfr
3. Shows all message blocked

It looks like you have browser.newtabpage.activity-stream.asrouter.userprefs.cfr.features set to false, so all groups that specify that for userPreferences are disabled (including cfr). The patch hasn't changed anything there, see attached image for the pre-patch behavior. So this looks expected to me, unless I'm misunderstanding the issue.

That's correct, in past it was easy to see browser.newtabpage.activity-stream.asrouter.userprefs.cfr.features is disabled by seeing message group enabled flag toggle along with disabled messages and a message Blocked by Group all in same view (as shown in pre-patch behavior and attached screenshots)

If it's a simpler change, will highly recommend splitting message group in its own page via left nav keeping General view focused on just messages and filtering messages by provider, template which is great thanks!

Comment 13

[Punam Dahiya [:pdahiya]]

Attachment: General Message Section.png

Comment 14

[Punam Dahiya [:pdahiya]]

Attachment: Message Group Section.png

Comment 15

[Shane Hughes [:aminomancer]]

(In reply to Punam Dahiya [:pdahiya] from comment #12)

My take by combining Messages and Message groups in a single page has added a lot of info to process pushing down the actual functionality specific to page below the fold.

General -> Showing all message JSONs , Message Group -> All messages in a group

Fair point. I suppose the space issue was less noticeable to me since I use a large monitor.

As for the Message Groups tab, it just seemed obsolete to me now that we have Filters. Especially if we added an "any group" filter that allows any group but excludes messages with no group. I guess we could still keep the Message Groups tab around though, not like it's hurting anything. As for pushing down the messages, we could also put the tables at the top somewhere else, or put them in a collapsed box that needs to be expanded. Both of them are pushing the messages down.

One idea is to add an "Advanced" tab or something along those lines, to replace the "Impressions" tab. Then put both tables in there, so it would include the Providers table, Groups table, Message Impressions, Screen Impressions, and Group Impressions. And then change the "General" tab to a "Messages" tab.

That's correct, in past it was easy to see browser.newtabpage.activity-stream.asrouter.userprefs.cfr.features is disabled by seeing message group enabled flag toggle along with disabled messages and a message Blocked by Group all in same view (as shown in pre-patch behavior and attached screenshots)

I'm not sure if I understand. I'm comparing my old nightly version (pre-patch) to the latest m-c (post-patch), side-by-side, and as far as I can tell, it works exactly the same way in both. There was never any display of the user pref values. Just the Groups table checkboxes, which still exist and are visible in your first screenshot. The only thing I changed in this respect is changing the label "blocked by group" to "message group blocked," which is less confusing IMO. Groups don't block messages, rather, groups are blocked by the user prefs.

Actually, I'm working on a patch to address some of your feedback, and in that patch I'm changing the label again, to "message group disabled" - to avoid confusing the concept of "blocking" with the concept of "disabling." Since what actually happens when you change the user pref is that group.enabled changes, there isn't actually a group block list like there is a message block list.

Aside from that, it's all the same. When a message group is disabled because of a user pref, its checkbox is unchecked in the groups table at the top, and all messages belonging to it are dimmed and disabled.

If it's a simpler change, will highly recommend splitting message group in its own page via left nav keeping General view focused on just messages and filtering messages by provider, template which is great thanks!

Is there a reason to remove the extra filters from the general tab? The main goal was to allow combining filters, which you couldn't do previously. You could only filter by one provider at a time on the general tab, and only one group at a time on the groups tab, and you couldn't filter by provider and group at the same time. If the general tab only allowed filtering by provider, that would make it much less powerful. It's the ability to say "show me feature_callout messages in the cfr group from the onboarding provider" that makes the Filters function more useful than having separate tabs. And that requires having filters for template, group, and provider.

Adding a separate groups tab is still possible, but what do you think about my other ideas to make more space for messages? Even if we add a groups tab, there's still one table at the top, pushing the messages down. One is better than two, but isn't zero better than one?

In my latest patch, I added some functionality to the groups table. In particular, I added checkboxes to the user prefs, so you can see which ones specifically are disabled, and you can even click the checkbox to change the pref right from the devtools, without needing to go to about:config. I also did something that requires the groups table to be on the same tab as the messages: the "message group blocked" message that shows when a message's group is disabled by pref, I changed that to an anchor link. So when you click it, it scrolls up to the groups table, so you can see why the group is disabled. I think it's a pretty handy feature but it wouldn't work as well if I moved the groups table to a separate tab. Because then you'd have to leave the tab you're on, losing track of the message you were looking at.

I'll see if I can collapse the tables until they're needed, and then I can show you what I've been working on and you can let me know what you think.