Closed Bug 1505704 Opened 10 months ago Closed 7 months ago

Flexbox-Toggle misses a tooltip

Categories

(DevTools :: Inspector, defect, P3)

63 Branch
defect

Tracking

(firefox67 fixed)

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: nachtigall, Assigned: championshuttler, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

Ripped off from bug 1494949. The flex Toogle should have a tooltip (title attribute) that says what it does.

I am surprised you do not have rules that catch those usuablity / accessibility error automatically. For instance I personally always use https://www.npmjs.com/package/eslint-plugin-jsx-a11y (only for React) or https://www.deque.com/axe/
See Also: → 1494949
Blocks: dt-flexbox
Priority: -- → P3

Thanks for reporting this! I think this is a good first bug and am willing to mentor anyone interested in it.

As Jens said, this can be solved by adding the title attribute to the input element rendered in devtools/client/inspector/flexbox/components/Header.js. Specifically, renderFlexboxHighlighterToggle is responsible for rendering the Flexbox Toggle component, which is where we can add a title attribute saying "Toggles Flexbox Highlighter".

Keywords: good-first-bug

Hi Micah, I would like to work on it can you help me getting started?

Thanks

Flags: needinfo?(tigleym)

Hi Shivam, thanks for picking this up! To get started, you can read through the "Getting Started" and "Contributing" sections at https://docs.firefox-dev.tools/. After that, feel free to ask any questions you have!

Flags: needinfo?(tigleym)

Hi Micah, so i am done with setting up gecko locally on my laptop , can you help me ahead how to do that , I am unable to figure it out.

Thanks

Hi Shivam! Could you tell me which part of the setup process you are having trouble with? That way we can figure out where to start.

Hi Micah, I am done with setup but i need help with how to add the title in renderFlexboxHighlighterToggle at devtools/client/inspector/flexbox/components/Header.js

Thanks

As we need to add title attribute to input element so can we do something like


  renderFlexboxHighlighterToggle() {
    // Don't show the flexbox highlighter toggle for the parent flex container of the
    // selected element.
    if (this.props.flexContainer.isFlexItemContainer) {
      return null;
    }

    return createElement(Fragment, null,
      dom.div({ className: "devtools-separator" }),
      dom.input({
        id: "flexbox-checkbox-toggle",
        className: "devtools-checkbox-toggle",
        checked: this.props.highlighted,
        onChange: this.onFlexboxCheckboxClick,
+       title: "Toggles Flexbox Highlighter",
        type: "checkbox",
      })
    );
  }
Flags: needinfo?(mtigley)

Yes! This is correct :)

Looking at this now, I realize we would need to ensure that our tooltip can be localized. I apologize for not mentioning this before. So in order to do this, we can do this by adding a unique translation key and the text Toggles Flexbox Highlighter to devtools/client/locales/en-US/layout.properties like so:

# LOCALIZATION NOTE (flexbox.togglesFlexboxHighlighter): The tooltip text for the Flexbox toggle button.
flexbox.togglesFlexboxHighlighter=Toggles Flexbox Highlighter

Then once we have done this, we can use getStr to use that translation key we created and retrieve the localized text. So, first, we would need to import getStr into devtools/client/inspector/flexbox/components/Header.js. Like this:

const { getStr } = require("devtools/client/inspector/layout/utils/l10n");

Now that we have getStr, we can use it like so:

>   renderFlexboxHighlighterToggle() {
>     // Don't show the flexbox highlighter toggle for the parent flex container of the
>     // selected element.
>     if (this.props.flexContainer.isFlexItemContainer) {
>       return null;
>     }
> 
>     return createElement(Fragment, null,
>       dom.div({ className: "devtools-separator" }),
>       dom.input({
>         id: "flexbox-checkbox-toggle",
>         className: "devtools-checkbox-toggle",
>         checked: this.props.highlighted,
>         onChange: this.onFlexboxCheckboxClick,
> +       title: getStr("flexbox.togglesFlexboxHighlighter"),
>         type: "checkbox",
>       })
>     );
>   }

Once this is done, our title attribute can be localized! Please let me know if something is unclear, or if you need any more help for this part. I will also make sure that you're assigned to this bug.

Also, in case you're interested, you can find more information on localization in the Firefox project here: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Create_localizable_strings

Flags: needinfo?(mtigley)
Assignee: nobody → shivams2799

Add tooltop in Flexbox toggle

Awesome thanks for the help @Micah, yeah I did this localizable string thing before , thank you so much for the help on Sunday as well :). Have a good day!

Created the patch.

Status: NEW → ASSIGNED
Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f2d79af5a356
Add tooltip in Flexbox toggle. r=mtigley
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

Please note that tooltips are usually written in infinitive mood to describe an action the user can choose, not "what it does". That would mean "Toggle Flexbox Highlighter" in this case.

https://transvision.mozfr.org/?recherche=toggle&repo=gecko_strings&sourcelocale=en-US&locale=en-US&search_type=strings

Also see https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Tooltips

Hi Shivam, would you be interested in making this fix? You will need to change the ID from "flexbox.togglesFlexboxHighlighter" to "flexbox.togglesFlexboxHighlighter2". We can't make changes to the existing localization without bumping the id so that it will be picked up by localization team.

Flags: needinfo?(shivams2799)

Hi Gabriel, thanks for asking sure I will send the patch soon :)

Thanks

Flags: needinfo?(shivams2799)
Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/882b1f8453a9
Fixes Flexbox-Toggle Tooltip spelling. r=mtigley
You need to log in before you can comment on or make changes to this bug.