1297302 - Fix tooltip of messageCloseButton in toolbox-notificationbox

Status: VERIFIED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[magicp]

Attachment: fix-tooltip-of-messageCloseButton-in-toolbox-notificationbox.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160822064441

Steps to reproduce:

1. Start Nightly
2. Go to about:home
3. Open DevTools > Canvas
4. Click Reload and Recording => wait until timeout => toolbox-notificationbox appears
5. Check tooltip of messageCloseButton


Actual results:

messageCloseButton tooltip is gone.

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=be6dbfceb79e570e6859200c2c5a8673f58c277e&tochange=bec48bfd05a8c6703aab63181441869a3dcaa4cc


Expected results:

Fix tooltip of messageCloseButton in toolbox-notificationbox

Comment 1

[Dalimil Hajek [:dalimil]]

So in client/shared/components/notification-box.js and function renderNotification(notification) - line 225 or so... When the close button is rendered in the rep component, the 'title' attribute is not specified -> we need to have:         
div({
  className: "messageCloseButton",
  title: 'This line is missing',
  onClick: this.close.bind(this, notification)}
) 
But I don't think we are passing the required tooltip string when creating a new notification, so at the moment I couldn't add it... Is it going to be a fixed string? I assume it also needs to be localized...

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I am not sure what's intended here, let's check with Honza who added the HTML notification bar.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Summary:

* Crate a new string 'notificationBox.closeTooltip' in components.properties, translated to 'notificationBox.closeTooltip'.
* Load string bundle in notification-box.js
const { LocalizationHelper } = require("devtools/shared/l10n");
const l10n = new LocalizationHelper("devtools/locale/components.properties");
* Create new props in NotificationBox component 'closeButtonTooltip: PropTypes.string'
* Set default value in getDefaultProps, closeButtonTooltip: l10n.getStr("notificationBox.closeTooltip")
* Render new 'title' attribute in reder method:  title: this.props.closeButtonTooltip,

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> Summary:
> 
> * Crate a new string 'notificationBox.closeTooltip' in
> components.properties, translated to 'notificationBox.closeTooltip'.
Sorry, translated to 'Close this message'

Honza

Comment 5

[Dalimil Hajek [:dalimil]]

Attachment: Bug1297302.patch

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8786260 [details] [diff] [review]
Bug1297302.patch

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

This is exactly what I had in mind, thanks! 

There is just one comment. Please fix and I'll R+ (assuming try is green)

Honza

::: devtools/client/shared/components/notification-box.js
@@ +53,5 @@
>        // Notification priority; see Priority Levels.
>        priority: PropTypes.number.isRequired,
>  
> +      // Message that should be shown when hovering over the close button
> +      closeButtonTooltip: PropTypes.string,

The prop should be outside of the `notifications` shape (it's prop of the NotificationBox not props of the Notification object).

Honza

Comment 7

[Dalimil Hajek [:dalimil]]

Attachment: Bug1297302.patch - rev2

I fixed the prop location.

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3850301404e5

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/733946d6ade3
Fix tooltip of messageCloseButton in toolbox-notificationbox. r=Honza

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/733946d6ade3

Comment 10

[magicp]

Verified in 51.