Visually indicate that the tool is not supported on the target type in the Options panel

RESOLVED FIXED in Firefox 24

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: Optimizer, Assigned: Optimizer)

Tracking

unspecified
Firefox 24
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 2 obsolete attachments)

For example, if the tool does not support remote target, disable the checkbox and have a tooltip saying that target is not supported
Will disable the checkbox with a tooltip saying that target not supported.
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Posted patch patch v1 (obsolete) — Splinter Review
Just calling isTargetSupported of the tool to see if target is supported. Adding a tooltip if not.
Attachment #754959 - Flags: review?(jwalker)
Comment on attachment 754959 [details] [diff] [review]
patch v1

Review of attachment 754959 [details] [diff] [review]:
-----------------------------------------------------------------

This could be confusing couldn't it? By default we have disabled the new (theoretical) 'appcache' panel. Appcache is disabled whenever the site isn't using appcache.

Since it's disabled by default, people don't know about the appcache panel. You're browsing the web and you read about the new appcache panel, and there are instructions on how to enable it, so you then have to hunt around for a site that supports appcache in order to enable it.

Or did I misunderstand?
Attachment #754959 - Flags: review?(jwalker)
(In reply to Joe Walker [:jwalker] from comment #3)
> Comment on attachment 754959 [details] [diff] [review]
> patch v1
> 
> Review of attachment 754959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This could be confusing couldn't it? By default we have disabled the new
> (theoretical) 'appcache' panel. Appcache is disabled whenever the site isn't
> using appcache.
> 
> Since it's disabled by default, people don't know about the appcache panel.
> You're browsing the web and you read about the new appcache panel, and there
> are instructions on how to enable it, so you then have to hunt around for a
> site that supports appcache in order to enable it.

umm. I have no idea what you are talking about.

> Or did I misunderstand?

This bug is for the following purpose :

Open a remote toolbox. Inspector is still not remote-able, so it will not be present in the toolbox. But when you go to options panel, the Inspector tool is still checked. The user is not confused, he tries to uncheck it and then check it again, but Inspector does not appear.

Same goes for any add-on that does not support either remote target or window target.
(In reply to Girish Sharma [:Optimizer] from comment #4)
> (In reply to Joe Walker [:jwalker] from comment #3)
> > Comment on attachment 754959 [details] [diff] [review]
> > patch v1
> > 
> > Review of attachment 754959 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This could be confusing couldn't it? By default we have disabled the new
> > (theoretical) 'appcache' panel. Appcache is disabled whenever the site isn't
> > using appcache.
> > 
> > Since it's disabled by default, people don't know about the appcache panel.
> > You're browsing the web and you read about the new appcache panel, and there
> > are instructions on how to enable it, so you then have to hunt around for a
> > site that supports appcache in order to enable it.
> 
> umm. I have no idea what you are talking about.
> 
> > Or did I misunderstand?
> 
> This bug is for the following purpose :
> 
> Open a remote toolbox. Inspector is still not remote-able, so it will not be
> present in the toolbox. But when you go to options panel, the Inspector tool
> is still checked. The user is not confused, he tries to uncheck it and then
> check it again, but Inspector does not appear.
> 
> Same goes for any add-on that does not support either remote target or
> window target.

So my understanding is correct.

Look at it another way - the options panel changes global options, but you're mixing target based information in with those global options.

We could perhaps indicate to the user with some text why the inspector doesn't appear ("Disabled for {reason}") or something. But altering the availability of global options based on locality doesn't make sense to me.
I see now what you are trying to say. So instead of disabling, should I just mark it with different color somehow ? Adding a text would take a lot of space, thus I reverted to using tooltips. How do you suggest to show a text alongside the checkbox.
(In reply to Girish Sharma [:Optimizer] from comment #6)
> I see now what you are trying to say. So instead of disabling, should I just
> mark it with different color somehow ? Adding a text would take a lot of
> space, thus I reverted to using tooltips. How do you suggest to show a text
> alongside the checkbox.

There's lots of space just to the right of the tool name isn't there?
Posted patch patch v2 (obsolete) — Splinter Review
Using similar approach as in bug 864249, added a double dagger mark after the non supported tool, and just below list of all tools, adding the explanation.
Attachment #754959 - Attachment is obsolete: true
Attachment #759941 - Flags: review?(jwalker)
Comment on attachment 759941 [details] [diff] [review]
patch v2

Review of attachment 759941 [details] [diff] [review]:
-----------------------------------------------------------------

Given bug 881807, we could use * rather than the double dagger thing!
Attachment #759941 - Flags: review?(jwalker) → review+
(In reply to Joe Walker [:jwalker] from comment #10)
> Given bug 881807, we could use * rather than the double dagger thing!

Agreed. Let me see if I can fix that quickly before this one to not have a key change.
Blocks: 881807
Posted patch patch v2.1Splinter Review
Using * instead of the dagger as star is free now (see bug 881807)
Attachment #759941 - Attachment is obsolete: true
Attachment #765560 - Flags: review+
Note that this bug and bug 881807 should be landed in the same push, this first then 881807
Whiteboard: [land-in-fx-team]
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/694bcbab0f2f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.