magicp Reporter
	Description • 8 years ago

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

	Dalimil Hajek [:dalimil] Assignee
	Comment 1 • 8 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 2 • 8 years ago

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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 3 • 8 years ago

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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 4 • 8 years ago

(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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 6 • 8 years ago

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

	Dalimil Hajek [:dalimil] Assignee
	Comment 7 • 8 years ago

I fixed the prop location.

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

	Pulsebot
	Comment 8 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/733946d6ade3
Fix tooltip of messageCloseButton in toolbox-notificationbox. r=Honza

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 8 years ago
bugherder

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

	magicp Reporter
	Comment 10 • 8 years ago

Verified in 51.