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)

Product:
DevTools
Component:
about:debugging
Type:
defect

Tracking

(firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: rickychien, Assigned: rickychien)

References

Details

Attachments

(5 files)

page-not-found-panel.png
63.71 KB, image/png
Details
unexpanded page-not-found.png
300.19 KB, image/png
hholmes
: feedback+
Details
expanded page-not-found.png
310.17 KB, image/png
hholmes
: feedback+
Details
page-not-found-with-error-msg.png
311.02 KB, image/png
hholmes
: ui-review+
Details
MozReview Request: Bug 1273076 - Show more details in about:debugging#invalid-hash r?jdescottes
58 bytes, text/x-review-board-request
jdescottes
: review+
Details 
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.
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)
I believe the idea is that it would expand and show the specific about:debugging error ("Unknown category %s").
Flags: needinfo?(hholmes)
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)
Both of them will show an underline once mouse hover on "Show more details" or "Hide more details"
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 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+
(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)
(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)
(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.
(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)
(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 :)
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?
(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)
(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)
(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)
Attachment #8753656 - Flags: feedback?(hholmes) → ui-review+
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/
Forgot to remove useless part, I've updated my patch again.
Attachment #8754189 - Flags: review?(jdescottes)
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.
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)
I've updated patch and .error-page .error-page-details is good to me.
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+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/86f8ed931685
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.