Closed
Bug 1273076
Opened 7 years ago
Closed 7 years ago
Show more details in about:debugging#invalid-hash
Categories
(DevTools :: about:debugging, defect, P3)
DevTools
about:debugging
Tracking
(firefox49 fixed)
RESOLVED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: rickychien, Assigned: rickychien)
References
Details
Attachments
(5 files)
UI improvement when navigating in about:debugging#invalid-hash. It might be nice to have a small, gray, underlined link with "Show more details". See also https://bugzilla.mozilla.org/show_bug.cgi?id=1268107#c15 and attachment.
Assignee | ||
Comment 1•7 years ago
|
||
Hi Helen, Any idea for displayed content after expanding "Show more details"? (As you said on https://bugzilla.mozilla.org/show_bug.cgi?id=1268107#c15)
Flags: needinfo?(hholmes)
Comment 2•7 years ago
|
||
I believe the idea is that it would expand and show the specific about:debugging error ("Unknown category %s").
Flags: needinfo?(hholmes)
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
Comment on attachment 8753173 [details] unexpanded page-not-found.png Patch will be submitted once bug 1268107 is landed. Helen, I uploaded unexpanded and expanded page-not-found panel. Need your feedback for current work.
Attachment #8753173 -
Flags: feedback?(hholmes)
Assignee | ||
Comment 6•7 years ago
|
||
Both of them will show an underline once mouse hover on "Show more details" or "Hide more details"
Comment 7•7 years ago
|
||
Comment on attachment 8753173 [details]
unexpanded page-not-found.png
Can we put an underline on the "Show more details" to show it's a link?
Attachment #8753173 -
Flags: feedback?(hholmes) → feedback+
Comment 8•7 years ago
|
||
Comment on attachment 8753174 [details]
expanded page-not-found.png
Ricky, are the majority of the error messages short like this, or do they vary in length?
Attachment #8753174 -
Flags: feedback+
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #7) > Comment on attachment 8753173 [details] > unexpanded page-not-found.png > > Can we put an underline on the "Show more details" to show it's a link? An underline will be displayed once mouse hover on "Show more details" or "Hide more details" (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #8) > Comment on attachment 8753174 [details] > expanded page-not-found.png > > Ricky, are the majority of the error messages short like this, or do they > vary in length? No, there are no error messages are thrown from system level. In most case, we expect a hash to be matched to a panel if user inputs a hash from url bar. I just put the #invalid-hash as error message so that indicates user there is no corresponding panel match their input.
Flags: needinfo?(hholmes)
Comment 10•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #9) > (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #8) > > Comment on attachment 8753174 [details] > > expanded page-not-found.png > > > > Ricky, are the majority of the error messages short like this, or do they > > vary in length? > > No, there are no error messages are thrown from system level. In most case, > we expect a hash to be matched to a panel if user inputs a hash from url > bar. I just put the #invalid-hash as error message so that indicates user > there is no corresponding panel match their input. Hm, new plan with the new info: 1. Let's remove the link and always show the error. 2. Let's reword the error to say: "<hash-in-url-bar>" does not exist!" An example: "#deevices does not exist!" Next, a question: How difficult would it be to guess what the user meant? "#deevices does not exist! Did you mean #devices?" << "#devices" being a link
Flags: needinfo?(hholmes) → needinfo?(rchien)
Comment 11•7 years ago
|
||
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #10) > Next, a question: > > How difficult would it be to guess what the user meant? > > "#deevices does not exist! Did you mean #devices?" << "#devices" being a link That's basically spell check; shouldn't be hard if we have scripting access to hunspell results in chrome JS. Would take an effort if we want to calculate our own in JS.
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #10) > Next, a question: > > How difficult would it be to guess what the user meant? > > "#deevices does not exist! Did you mean #devices?" << "#devices" being a link Yeah, it's not so hard to implement by leveraging edit distance algorithm. My proposal is: For edit distance within 3 (#deeeevices which has extra 3 "e" will auto-correct to devices) and display: "#deeeevices does not exist! Did you mean #devices?" << "#devices" being a link For edit distance above 3 (#deeeeeevices which will not ) "#deeeeeevices does not exist!
Flags: needinfo?(rchien)
Attachment #8753656 -
Flags: feedback?(hholmes)
Comment 13•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #12) > (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #10) > > Next, a question: > > > > How difficult would it be to guess what the user meant? > > > > "#deevices does not exist! Did you mean #devices?" << "#devices" being a link > > Yeah, it's not so hard to implement by leveraging edit distance algorithm. > No it's hard, but I would worry the edit distance algorithm only serves this one example? That said, as long as you have an agreement with Helen on what we want, it's fine :)
Assignee | ||
Comment 14•7 years ago
|
||
On the other hand, navigating by inputing a hash on url bar is not a pretty straightforward way to go. I can understand that this error page is an error indicator for someone who entered #invalid-hash accidentally. A recommendation for this would be too much in most case, so I prefer to give user a simple message "#invalid-hash does not exist!". Helen, what do you think?
Comment 15•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #14) > A recommendation for this would be too much in most case, so I prefer to > give user a simple message "#invalid-hash does not exist!". Helen, what do > you think? I agree, and think we should keep things simple here. The use case is not likely to happen very often anyway. Regarding the different types of errors we could get, I remember :janx mentioning that some about:debugging categories could become disabled depending on the context. So in the future, I can imagine two kind of errors: - hash matches no existing about:debugging category - valid hash, but not available for X / Y However for the second one, it's not clear to me if this should be displayed in a generic error page, or as a warning in the existing category page. For now, I think we should design this error page for the one error we handle today. Jan: What do you think? (adding ni? to Helen as well for the question from comment 14)
Flags: needinfo?(janx)
Flags: needinfo?(hholmes)
Comment 16•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #15) > I agree, and think we should keep things simple here. The use case is not > likely to happen very often anyway. Seconded, the "user types a bad hash" use case probably won't happen very often. Note: :bigben pointed out to me that about:preferences simply redirects users to #general on an invalid hash. However, I still think in our case it's better to show an error: For example, if a user mistakenly types #serviceworkers, and gets redirected to #addons, that's confusing. If instead we show a 404-type message like "#serviceworkers doesn't exist", the user might recognize that they were actually looking for #workers. > Regarding the different types of errors we could get, I remember :janx > mentioning that some about:debugging categories could become disabled > depending on the context. > > So in the future, I can imagine two kind of errors: > - hash matches no existing about:debugging category > - valid hash, but not available for X / Y > > However for the second one, it's not clear to me if this should be displayed > in a generic error page, or as a warning in the existing category page. > > For now, I think we should design this error page for the one error we > handle today. I completely agree, and for the future it's a good idea to show the second error (warning/error about a target type not being supported by the current runtime) in the relevant panel, instead of showing the generic "not found" page.
Flags: needinfo?(janx)
Comment 17•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #14) > On the other hand, navigating by inputing a hash on url bar is not a pretty > straightforward way to go. I can understand that this error page is an error > indicator for someone who entered #invalid-hash accidentally. > > A recommendation for this would be too much in most case, so I prefer to > give user a simple message "#invalid-hash does not exist!". Helen, what do > you think? I'm fine with that. If the suggestion message is overkill, let's go with what's simple.
Flags: needinfo?(hholmes)
Updated•7 years ago
|
Attachment #8753656 -
Flags: feedback?(hholmes) → ui-review+
Assignee | ||
Comment 18•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53824/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53824/
Attachment #8754189 -
Flags: review?(jdescottes)
Assignee | ||
Comment 19•7 years ago
|
||
Comment on attachment 8754189 [details] MozReview Request: Bug 1273076 - Show more details in about:debugging#invalid-hash r?jdescottes Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53824/diff/1-2/
Assignee | ||
Comment 20•7 years ago
|
||
Forgot to remove useless part, I've updated my patch again.
Updated•7 years ago
|
Attachment #8754189 -
Flags: review?(jdescottes)
Comment 21•7 years ago
|
||
Comment on attachment 8754189 [details] MozReview Request: Bug 1273076 - Show more details in about:debugging#invalid-hash r?jdescottes https://reviewboard.mozilla.org/r/53824/#review50622 Looks very good thanks! The UI is as expected, and gives a nice additional feedbakck to the user. Not clearing my review flag in case we need to discuss my comments. ::: devtools/client/aboutdebugging/aboutdebugging.css:148 (Diff revision 2) > + flex-direction: column; > width: 100%; > height: 100%; > } > + > +.error { I feel like "error" is too generic as a classname here (particularly to apply a color: gray). I have some suggestions below, but I am not great with CSS naming, so feel free to go with anything you like. We could specialize the selector by prefixing it with .page-not-found => ".page-not-found .error" We could also change the classname directly "page-not-found-details" Or maybe change "page-not-found" to "error-page". Then we could have ".error-page" and ".error-page-details" ? ::: devtools/client/aboutdebugging/components/aboutdebugging.js:88 (Diff revision 2) > let panel; > > if (selectedPanel) { > panel = selectedPanel.component({ client, id: selectedPanel.id }); > } else { > + let errorMsg = Strings.GetStringFromName("doesNotExist"); To make this more flexible I think we should use a pattern here: in the properties file: doesNotExist = #%S does not exist! in aboutdebugging.js: Strings.formatStringFromName("doesNotExist", [selectedPanelId], 1); This way if the ordering of the message has to change, only the properties file has to be updated. Also if different locales need a different order, this can be handled as well. ::: devtools/client/aboutdebugging/components/aboutdebugging.js:94 (Diff revision 2) > panel = ( > dom.div({ className: "page-not-found" }, > dom.h1({ className: "header-name" }, > Strings.GetStringFromName("pageNotFound") > + ), > + dom.h4({ className: "error" }, `#${selectedPanelId} ${errorMsg}!`) See previous comment, let's move all the pattern to the properties file. ::: devtools/client/locales/en-US/aboutdebugging.properties:29 (Diff revision 2) > otherWorkers = Other Workers > > tabs = Tabs > > pageNotFound = Page not found > +doesNotExist = does not exist See comment above: let's use doesNotExist = #%S does not exist! And add a comment just above explaining what %S is in this context, and when the message is displayed.
Updated•7 years ago
|
Attachment #8754189 -
Flags: feedback+
Assignee | ||
Comment 22•7 years ago
|
||
Comment on attachment 8754189 [details] MozReview Request: Bug 1273076 - Show more details in about:debugging#invalid-hash r?jdescottes Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53824/diff/2-3/
Attachment #8754189 -
Flags: feedback+ → review?(jdescottes)
Assignee | ||
Comment 23•7 years ago
|
||
I've updated patch and .error-page .error-page-details is good to me.
Comment 24•7 years ago
|
||
Comment on attachment 8754189 [details] MozReview Request: Bug 1273076 - Show more details in about:debugging#invalid-hash r?jdescottes https://reviewboard.mozilla.org/r/53824/#review50856 Looks good to me! Thanks a lot for handling this follow up. r+ with a green try.
Attachment #8754189 -
Flags: review?(jdescottes) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 25•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/86f8ed931685
Keywords: checkin-needed
Comment 26•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #21) > > pageNotFound = Page not found > > +doesNotExist = does not exist > > See comment above: let's use > doesNotExist = #%S does not exist! > > And add a comment just above explaining what %S is in this context, and when > the message is displayed. With no comment (as landed) one can expect bad translation to arrive.
Comment 27•7 years ago
|
||
(In reply to Stefan Plewako [:stef] from comment #27) > (In reply to Julian Descottes [:jdescottes] from comment #21) > > > pageNotFound = Page not found > > > +doesNotExist = does not exist > > > > See comment above: let's use > > doesNotExist = #%S does not exist! > > > > And add a comment just above explaining what %S is in this context, and when > > the message is displayed. > > With no comment (as landed) one can expect bad translation to arrive. I missed that in the last review, thanks for catching this. I will log a follow up to add comments to aboutdebugging.properties.
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/86f8ed931685
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 29•6 years ago
|
||
I have reproduced this bug with Nightly 49.0a1 (2016-05-16) on Windows 8 , 64 Bit ! THis bug's fix is verified with Firefox release 49.0 Build ID : 20160916101415 User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0 [bugday-20160928]
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•