Closed Bug 1112304 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: General, defect)

x86
All
defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
e10s m6+ ---

People

(Reporter: mconley, Assigned: mossop)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
Flags: firefox-backlog+
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-
Attached image 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)
Attached image Non-retina screenshot
Non-retina version
Attachment #8549262 - Attachment description: Screenshot 2015-01-14 16.04.01.png → Retina screenshot
Attachment #8550383 - Attachment is obsolete: true
Attachment #8550383 - Flags: review?(smacleod)
Attachment #8550383 - Flags: review?(dao)
Attached patch patch (obsolete) — Splinter Review
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-
Attached patch patch rev 2 (obsolete) — Splinter Review
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-
Attached image Screenshot 2015-01-20 09.56.40.png (obsolete) —
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)
Attached patch patch rev 3Splinter Review
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+
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
https://hg.mozilla.org/mozilla-central/rev/666746987b4b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Depends on: 1129266
Attachment #8549262 - Flags: ui-review?(mmaslaney)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: