1273076 - Show more details in about:debugging#invalid-hash

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect, P3)

Description

[Ricky Chien [:rickychien] (inactive)]

Attachment: page-not-found-panel.png

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.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

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)

Comment 2

[Helen V. Holmes (:helenvholmes) (:✨)]

I believe the idea is that it would expand and show the specific about:debugging error ("Unknown category %s").

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Attachment: unexpanded page-not-found.png

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Attachment: expanded page-not-found.png

Comment 5

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Both of them will show an underline once mouse hover on "Show more details" or "Hide more details"

Comment 7

[Helen V. Holmes (:helenvholmes) (:✨)]

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?

Comment 8

[Helen V. Holmes (:helenvholmes) (:✨)]

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?

Comment 9

[Ricky Chien [:rickychien] (inactive)]

(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.

Comment 10

[Helen V. Holmes (:helenvholmes) (:✨)]

(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

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Attachment: page-not-found-with-error-msg.png

(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!

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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 :)

Comment 14

[Ricky Chien [:rickychien] (inactive)]

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

[Julian Descottes [:jdescottes]]

(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)

Comment 16

[Jan Keromnes [:janx]]

(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.

Comment 17

[Helen V. Holmes (:helenvholmes) (:✨)]

(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.

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Attachment: MozReview Request: Bug 1273076 - Show more details in about:debugging#invalid-hash r?jdescottes

Review commit: https://reviewboard.mozilla.org/r/53824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53824/

Comment 19

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 20

[Ricky Chien [:rickychien] (inactive)]

Forgot to remove useless part, I've updated my patch again.

Comment 21

[Julian Descottes [: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 22

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 23

[Ricky Chien [:rickychien] (inactive)]

I've updated patch and .error-page .error-page-details is good to me.

Comment 24

[Julian Descottes [: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/#review50856

Looks good to me! Thanks a lot for handling this follow up.

r+ with a green try.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/86f8ed931685

Comment 26

[Stefan Plewako [:stef]]

(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

[Julian Descottes [:jdescottes]]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86f8ed931685

Comment 29

[Md Maruf Rahman]

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]