Style Editor should have a page loading placeholder

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Developer Tools
--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: cedricv, Assigned: cedricv)

Tracking

unspecified
Firefox 12
Points:
---

Firefox Tracking Flags

(firefox11- verified)

Details

(Whiteboard: [styleeditor][qa+])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Quickly opening the Style Editor while the page is still loading shows the "This page has no style sheet" placeholder.

There should rather be a "The page is is still loading" placeholder as soon it is known the page contains style sheets.
(Assignee)

Comment 1

6 years ago
Implemented in add-on v0.5.4 :
https://github.com/neonux/StyleEditor/commit/cb91c59cccecd2092dbf1a0272ba36e35d6e2917

Browser patch after landing.
(Assignee)

Comment 2

6 years ago
It just shows a throbber in the center of the (empty) style sheet list [if and while the page is still loading].
I thought that there's no need to be more verbose.

Any objection/opinion Shorlander?

We'll probably need a DevTools-specific throbber too at some point (to match with the dark design).
(Assignee)

Comment 3

5 years ago
Created attachment 588045 [details] [diff] [review]
Display a throbber while the page is still loading instead of "this page has no stylesheet"
Assignee: nobody → cedricv
Status: NEW → ASSIGNED
Attachment #588045 - Flags: review?
(Assignee)

Comment 4

5 years ago
This might be worth an aurora approval request (?).
The currently landed behavior can be quite confusing for users on slow connections.
(Assignee)

Updated

5 years ago
Attachment #588045 - Flags: review? → review?(rcampbell)
Comment on attachment 588045 [details] [diff] [review]
Display a throbber while the page is still loading instead of "this page has no stylesheet"

+    // (re)enable UI
+    let matches = this._root.querySelectorAll("toolbarbutton,input,select");
+    for (let i = 0; i < matches.length; ++i) {
+      matches[i].removeAttribute("disabled");
+    }

hm! OK.

...

CSS looks ok to me. Yay, throbbers!
Attachment #588045 - Flags: review?(rcampbell) → review+
feel free to send this through try. If you think it'll pass without one, add [land-in-fx-team].

After landing, we can request aurora approval.
Comment on attachment 588045 [details] [diff] [review]
Display a throbber while the page is still loading instead of "this page has no stylesheet"

>--- a/browser/themes/pinstripe/devtools/splitview.css
>+++ b/browser/themes/pinstripe/devtools/splitview.css

>+.loading .splitview-nav-container {
>+  background-image: url(chrome://browser/skin/tabbrowser/loading.png);

Don't reference images in tabbrowser/ here. Either copy it or use chrome://global/skin/icons/loading_16.png.
(Assignee)

Comment 8

5 years ago
Created attachment 589093 [details] [diff] [review]
patch v2 using global throbber instead of tabbrowser's
Attachment #588045 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [styleeditor] → [styleeditor][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/4c3923ea4a2d
Whiteboard: [styleeditor][land-in-fx-team] → [styleeditor][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4c3923ea4a2d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [styleeditor][fixed-in-fx-team] → [styleeditor]
Target Milestone: --- → Firefox 12
tracking-firefox11: --- → ?
Comment on attachment 589093 [details] [diff] [review]
patch v2 using global throbber instead of tabbrowser's

[Approval Request Comment]
Regression caused by (bug #): New Feature. Not a regression.
User impact if declined: Users won't know if their style sheets are loading or not.
Testing completed (on m-c, etc.): on m-c.
Risk to taking this patch (and alternatives if risky): Low. Mostly styling. Alternative is users don't get this functionality until fx 12.

Hey lookit the little auto-created bugzilla form! Neat.
Attachment #589093 - Flags: approval-mozilla-aurora?
Comment on attachment 589093 [details] [diff] [review]
patch v2 using global throbber instead of tabbrowser's

[Triage Comment]
Low risk, prevents user confusion with new feature. Approved for Aurora 11.
Attachment #589093 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b6c3fb586ba5
status-firefox11: --- → fixed
Will not apply because of missing dependent patch. Not sure what to do about that.

adding placeholder to series file
applying placeholder
patching file browser/devtools/styleeditor/StyleEditorChrome.jsm
Hunk #1 FAILED at 150
Hunk #2 FAILED at 293
2 out of 2 hunks FAILED -- saving rejects to file browser/devtools/styleeditor/StyleEditorChrome.jsm.rej
patching file browser/devtools/styleeditor/splitview.css
Hunk #1 FAILED at 40
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/styleeditor/splitview.css.rej
patching file browser/devtools/styleeditor/styleeditor.xul
Hunk #1 FAILED at 48
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/styleeditor/styleeditor.xul.rej
patching file browser/devtools/styleeditor/test/browser_styleeditor_loading.js
Hunk #1 FAILED at 12
Hunk #2 FAILED at 29
2 out of 2 hunks FAILED -- saving rejects to file browser/devtools/styleeditor/test/browser_styleeditor_loading.js.rej
patching file browser/themes/gnomestripe/devtools/splitview.css
Hunk #1 FAILED at 35
1 out of 1 hunks FAILED -- saving rejects to file browser/themes/gnomestripe/devtools/splitview.css.rej
patching file browser/themes/gnomestripe/devtools/styleeditor.css
Hunk #1 FAILED at 30
1 out of 1 hunks FAILED -- saving rejects to file browser/themes/gnomestripe/devtools/styleeditor.css.rej
patching file browser/themes/pinstripe/devtools/splitview.css
Hunk #1 FAILED at 35
1 out of 1 hunks FAILED -- saving rejects to file browser/themes/pinstripe/devtools/splitview.css.rej
patching file browser/themes/pinstripe/devtools/styleeditor.css
Hunk #1 FAILED at 30
1 out of 1 hunks FAILED -- saving rejects to file browser/themes/pinstripe/devtools/styleeditor.css.rej
patching file browser/themes/winstripe/devtools/splitview.css
Hunk #1 FAILED at 35
1 out of 1 hunks FAILED -- saving rejects to file browser/themes/winstripe/devtools/splitview.css.rej
patching file browser/themes/winstripe/devtools/styleeditor.css
Hunk #1 FAILED at 30
1 out of 1 hunks FAILED -- saving rejects to file browser/themes/winstripe/devtools/styleeditor.css.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh placeholder
disregard that. already in. because awesom.
Whiteboard: [styleeditor] → [styleeditor][qa+]

Updated

5 years ago
tracking-firefox11: ? → -
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0

Still able to catch the issue if opening very quick the style editor (SHIFT+F7) after pressing ENTER in the location bar: www.cnn.com
(Assignee)

Comment 17

5 years ago
(In reply to Paul Silaghi [QA] from comment #16)
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0
> Still able to catch the issue if opening very quick the style editor
> (SHIFT+F7) after pressing ENTER in the location bar: www.cnn.com

Hmm, can you please open a new bug for this with detailed steps to reproduce?
Also, are you able to repro on every platform?
New bug 725222 filed.
Marking this as verified fixed on Firefox 11, since the editor works fine in normal conditions:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0
status-firefox11: fixed → verified
You need to log in before you can comment on or make changes to this bug.