Last Comment Bug 701635 - Style Editor should have a page loading placeholder
: Style Editor should have a page loading placeholder
Status: RESOLVED FIXED
[styleeditor][qa+]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: Firefox 12
Assigned To: Cedric Vivier [:cedricv]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-10 23:36 PST by Cedric Vivier [:cedricv]
Modified: 2012-02-08 00:38 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
verified


Attachments
Display a throbber while the page is still loading instead of "this page has no stylesheet" (12.52 KB, patch)
2012-01-12 08:40 PST, Cedric Vivier [:cedricv]
rcampbell: review+
Details | Diff | Review
patch v2 using global throbber instead of tabbrowser's (12.45 KB, patch)
2012-01-16 20:17 PST, Cedric Vivier [:cedricv]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Cedric Vivier [:cedricv] 2011-11-10 23:36:45 PST
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.
Comment 1 Cedric Vivier [:cedricv] 2011-11-11 01:35:29 PST
Implemented in add-on v0.5.4 :
https://github.com/neonux/StyleEditor/commit/cb91c59cccecd2092dbf1a0272ba36e35d6e2917

Browser patch after landing.
Comment 2 Cedric Vivier [:cedricv] 2011-11-11 01:44:26 PST
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).
Comment 3 Cedric Vivier [:cedricv] 2012-01-12 08:40:53 PST
Created attachment 588045 [details] [diff] [review]
Display a throbber while the page is still loading instead of "this page has no stylesheet"
Comment 4 Cedric Vivier [:cedricv] 2012-01-12 09:03:27 PST
This might be worth an aurora approval request (?).
The currently landed behavior can be quite confusing for users on slow connections.
Comment 5 Rob Campbell [:rc] (:robcee) 2012-01-16 14:42:29 PST
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!
Comment 6 Rob Campbell [:rc] (:robcee) 2012-01-16 14:44:16 PST
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 7 Dão Gottwald [:dao] 2012-01-16 15:18:19 PST
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.
Comment 8 Cedric Vivier [:cedricv] 2012-01-16 20:17:50 PST
Created attachment 589093 [details] [diff] [review]
patch v2 using global throbber instead of tabbrowser's
Comment 9 Rob Campbell [:rc] (:robcee) 2012-01-19 07:09:35 PST
https://hg.mozilla.org/integration/fx-team/rev/4c3923ea4a2d
Comment 10 Tim Taubert [:ttaubert] 2012-01-19 15:45:56 PST
https://hg.mozilla.org/mozilla-central/rev/4c3923ea4a2d
Comment 11 Rob Campbell [:rc] (:robcee) 2012-01-23 07:13:38 PST
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.
Comment 12 Alex Keybl [:akeybl] 2012-01-23 09:12:27 PST
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.
Comment 13 Rob Campbell [:rc] (:robcee) 2012-01-24 07:15:34 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/b6c3fb586ba5
Comment 14 Rob Campbell [:rc] (:robcee) 2012-01-26 11:54:28 PST
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
Comment 15 Rob Campbell [:rc] (:robcee) 2012-01-26 12:09:31 PST
disregard that. already in. because awesom.
Comment 16 Paul Silaghi, QA [:pauly] 2012-02-07 06:26:15 PST
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
Comment 17 Cedric Vivier [:cedricv] 2012-02-07 06:35:52 PST
(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?
Comment 18 Paul Silaghi, QA [:pauly] 2012-02-08 00:38:29 PST
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

Note You need to log in before you can comment on or make changes to this bug.