Flesh out about:tabcrashed to use all strings that final about:tabcrashed spec will use.

RESOLVED FIXED in Firefox 38

Status

()

Firefox
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mconley, Assigned: mossop)

Tracking

Trunk
Firefox 38
x86
All
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(e10sm6+)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
about:tabcrashed is the only new UI with strings shipping with e10s, and it has strings. We need to get those strings in and landed. This should probably happen before all of the styling work in bug 1110511.
(Assignee)

Updated

3 years ago
Flags: firefox-backlog+
(Assignee)

Comment 1

3 years ago
I might as well do this in bug 1109650
Assignee: mconley → dtownsend
Status: NEW → ASSIGNED
Iteration: --- → 38.1 - 26 Jan
Points: --- → 2
Depends on: 1109650
Flags: qe-verify-
(Assignee)

Comment 2

3 years ago
Created attachment 8549262 [details]
Retina screenshot

In this bug I've implemented everything from the spec in bug 1110511 apart from the form to enter details for the crash report. Here is what things look like, can you let me know if it looks right?

For the tab icon overlay there isn't a lot of room to play with without expanding the tab, the overlay is offset 5px vertically and 6px horizontally here.

The in-content styles are in use and essentially unaltered so font sizes, buttons etc. should match those in the net error pages and elsewhere that uses in-content styles. The exception is the page icon which we can align/size individually so I tried to match the mockup.

This is a retina screenshot.
Attachment #8549262 - Flags: ui-review?(mmaslaney)
(Assignee)

Comment 3

3 years ago
Created attachment 8549263 [details]
Non-retina screenshot

Non-retina version
(Assignee)

Updated

3 years ago
Attachment #8549262 - Attachment description: Screenshot 2015-01-14 16.04.01.png → Retina screenshot
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

3 years ago
Attachment #8550383 - Attachment is obsolete: true
Attachment #8550383 - Flags: review?(smacleod)
Attachment #8550383 - Flags: review?(dao)
(Assignee)

Comment 6

3 years ago
Created attachment 8550387 [details] [diff] [review]
patch

Assuming that the UX review might only mean a few pixel related changes might as well get on with the code review. This uses in-content styles for the tab crash page and adds an overlay to the favicon for crashed tabs. Also adds support for closing the crashed tab. The strings here refer to being able to restore all tabs, that will be implemented and landed at the same time in bug 1109650 to avoid l10n churn.

I've run talos tests and verified that adding the deck to the tab doesn't harm performance.
Attachment #8550387 - Flags: review?(dao)
Comment on attachment 8550387 [details] [diff] [review]
patch

I think adding tab-icon-stack is overkill. Seems like tab-icon-overlay could just be a sibling to tab-icon-image and overlay the icon with negative margins. Am I missing something?

>+++ b/browser/base/content/aboutTabCrashed.css
>@@ -0,0 +1,12 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#reportSent, #report-box {

nit: new line after ,
Attachment #8550387 - Flags: review?(dao) → review-
(Assignee)

Comment 8

3 years ago
Created attachment 8550478 [details] [diff] [review]
patch rev 2

Yeah that's probably better, didn't realise that negative margins worked well in XUL
Attachment #8550387 - Attachment is obsolete: true
Attachment #8550478 - Flags: review?(dao)
Comment on attachment 8550478 [details] [diff] [review]
patch rev 2

>--- a/browser/base/content/tabbrowser.css
>+++ b/browser/base/content/tabbrowser.css

> .tab-icon-image:not([src]):not([pinned]),
> .tab-throbber:not([busy]),
>-.tab-throbber[busy] + .tab-icon-image {
>+.tab-throbber[busy] + .tab-icon-image,
>+.tab-throbber[busy] + .tab-icon-overlay {
>   display: none;
> }

This doesn't work, as tab-throbber and tab-icon-overlay aren't adjacent. Probably best to inherit the 'busy' attribute to tab-icon-image and tab-icon-overlay.

>+          <xul:image xbl:inherits="crashed"
>+                     anonid="tab-icon-overlay"
>+                     class="tab-icon-overlay"
>+                     validate="never"
>+                     role="presentation"/>

validate="never" doesn't make sense here and anonid is unused, can be removed.

>--- a/browser/components/sessionstore/test/browser_crashedTabs.js
>+++ b/browser/components/sessionstore/test/browser_crashedTabs.js

>+Services.prefs.setBoolPref("browser.tabs.animate", false)
>+registerCleanupFunction(() => {
>+  Services.prefs.clearUserPref("browser.tabs.animate")
>+});

nit: missing semicolons

>+/* Hide the actual checkbox */
>+html|input[type="checkbox"] {
>+  opacity: 0;
>+  position: absolute;
>+}

For keyboard accessibility we'll need to draw some custom focus ring here since the real checkbox is hidden.
Attachment #8550478 - Flags: review?(dao) → review-
(Assignee)

Comment 10

2 years ago
Created attachment 8551949 [details]
Screenshot 2015-01-20 09.56.40.png

I've come across a bug with non-selected tabs on OSX. There we make the tab icon slightly transparent (http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#3095). For some reason this causes the tab icon to bleed through the overlay, even though that is completely opaque. Toggling that rule off in the toolbox solves it. This wasn't a problem with the stack where it was the stack that was made transparent.

Any idea why this happens and how to solve it?
Flags: needinfo?(dao)
It looks like the reduced opacity puts the icon _above_ the overlay, as per https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Understanding_z_index/The_stacking_context. In that case I think you should be able to set isolation:isolate or opacity:.999 on the overlay to put it back on top...
Flags: needinfo?(dao)
(Assignee)

Comment 12

2 years ago
Created attachment 8552004 [details] [diff] [review]
patch rev 3

That was it. setting isolation didn't work so using opacity here. I also saw that "will-change: opacity" worked too, not sure which is better to use.

Also discovered a bug with tabs with no favicon. In this case the icon is hidden so we have to apply additional margin on the overlay to get it into the right place.

> >+/* Hide the actual checkbox */
> >+html|input[type="checkbox"] {
> >+  opacity: 0;
> >+  position: absolute;
> >+}
> 
> For keyboard accessibility we'll need to draw some custom focus ring here
> since the real checkbox is hidden.

I'd like to defer on this, this is a common problem for checkboxes in the in-content UI and we don't yet have a spec from UX for what to do. I'd rather add this as one of the things to be handled by bug 1008171.
Attachment #8550478 - Attachment is obsolete: true
Attachment #8551949 - Attachment is obsolete: true
Attachment #8552004 - Flags: review?(dao)
Comment on attachment 8552004 [details] [diff] [review]
patch rev 3

>+.tab-icon-overlay {
>+  opacity: 0.9999;
>+}

Probably worth a comment.

>+.tab-icon-image:not([src]):not([pinned]) + .tab-icon-overlay {
>+  -moz-margin-start: 6px;
>+}

Maybe we shouldn't hide the icon when the overlay is shown? Otherwise the overlay could be confused with the icon, and further an icon could be confused with the overlay (i.e. web pages could spoof the overlay by using it as their icon).
Attachment #8552004 - Flags: review?(dao) → review+

Updated

2 years ago
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
(Assignee)

Comment 14

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/666746987b4b
https://hg.mozilla.org/mozilla-central/rev/666746987b4b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38

Updated

2 years ago
Depends on: 1129266

Updated

2 years ago
Attachment #8549262 - Flags: ui-review?(mmaslaney)
You need to log in before you can comment on or make changes to this bug.