Closed Bug 1425538 Opened 7 years ago Closed 7 years ago

Move the NotificationBox as a sibling of the ConsoleOutput

Categories

(DevTools :: Console, enhancement, P1)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox59 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: nchevobbe, Assigned: Honza)

References

Details

(Whiteboard: [boogaloo-mvp])

Attachments

(1 file)

Bug 1425538 - Use NotificationBox as regular React element;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Assignee: nobody → odvarko
Priority: -- → P2
Comment on attachment 8974729 [details] Bug 1425538 - Use NotificationBox as regular React element; @Brian, I am attaching wip patch for some feedback - The NotificationBox is now used as regular React element - There are new notification actions (like append and remove) - There is new reducer with notificattions - I also adopted the existing NotificationBox to work with a reducer. - There is a new App component representing the root of the panel (and responsible for updating the notification box) - Also the way how self-xss attacks are handled has been changed. The code needs cleanup and try needs to be green, but some feedback would already be useful! Honza
Attachment #8974729 - Flags: feedback?(bgrinstead)
Comment on attachment 8974729 [details] Bug 1425538 - Use NotificationBox as regular React element; This looks really nice to me since it's pulling more logic out of webconsole.js/jsterm.js and into the component. I'd like Nicolas to review this, but to me this looks like the right direction so would be worth starting to tackle tests etc.
Attachment #8974729 - Flags: feedback?(bgrinstead) → feedback+
Comment on attachment 8974729 [details] Bug 1425538 - Use NotificationBox as regular React element; https://reviewboard.mozilla.org/r/243102/#review249524 ::: devtools/client/shared/components/NotificationBox.js:247 (Diff revision 3) > /** > * Render the top (highest priority) notification. Only one > * notification is rendered at a time. > */ > render() { > - let notification = this.state.notifications.first(); > + const notifications = this.props.notifications || this.state.notifications; could this cause some issues ? like, the state is updated, but the component still has props.notifications, so it does not reflect the new state ? ::: devtools/client/shared/components/NotificationBox.js:249 (Diff revision 3) > + const content = notification ? > this.renderNotification(notification) : > null; > > - return div({className: "notificationbox"}, > + return div({ > + className: "notificationbox", > + id: this.props.id}, > content > ); we could directly return null if `notification` is falsy maybe ? If the content is null, there's no need for us to render an empty div. ::: devtools/client/shared/components/NotificationBox.js:285 (Diff revision 3) > + // Custom image URL is not supported yet. > + if (image) { > + throw new Error("Custom image URL is not supported yet"); > + } not related to your patch, but this feels weird to me. Should we simply remove the image prop ? ::: devtools/client/webconsole/actions/notifications.js:37 (Diff revision 3) > + * notification list. > + */ > +function removeNotification(value) { > + return { > + type: REMOVE_NOTIFICATION, > + value: value, nit: can be replaced with `value` only ::: devtools/client/webconsole/components/App.js:69 (Diff revision 3) > + dispatch(actions.appendNotification(SELF_XSS_MSG, > + "selfxss-notification", null, > + PriorityLevels.PRIORITY_WARNING_HIGH, null, > + function(eventType) { > + // Cleanup function if notification is closed by the user. > + if (eventType == "removed") { > + inputField.removeEventListener("keyup", pasteKeyUpHandler); > + dispatch(actions.removeNotification("selfxss-notification")); > + } > + } > + )); nit: this is a bit hard to parse, could we pass an object to the action instead so the value we pass make more sense to the reader ? ::: devtools/client/webconsole/components/App.js:74 (Diff revision 3) > + dispatch(actions.appendNotification(SELF_XSS_MSG, > + "selfxss-notification", null, > + PriorityLevels.PRIORITY_WARNING_HIGH, null, > + function(eventType) { > + // Cleanup function if notification is closed by the user. > + if (eventType == "removed") { looking at the NotificationBox code, it looks like that we only execute the callback with the "removed" eventType. What do you think of not passing the eventType when calling this callback, and rename the props in the NotificationBox to something like `onNotificationRemoved` ? ::: devtools/client/webconsole/components/App.js:83 (Diff revision 3) > + } > + )); > + > + // Remove notification automatically when the user > + // types "allows pasting". > + function pasteKeyUpHandler(event2) { nit: i think event2 is not used, we can remove it ::: devtools/client/webconsole/components/App.js:88 (Diff revision 3) > + function pasteKeyUpHandler(event2) { > + let value = inputField.value || inputField.textContent; > + if (value.includes(SELF_XSS_OK)) { > + dispatch(actions.removeNotification("selfxss-notification")); > + inputField.removeEventListener("keyup", pasteKeyUpHandler); > + WebConsoleUtils.usageCount = WebConsoleUtils.CONSOLE_ENTRY_THRESHOLD; So, looking at the code below, it seems like we use this as a way to indicate if we should add the onPast even listener. Since it looks like a state to me, I think this should go into the redux store. ::: devtools/client/webconsole/components/App.js:107 (Diff revision 3) > + let onPaste = hud.isBrowserConsole ? undefined : this.onPaste; > + > + const { > + usageCount, > + CONSOLE_ENTRY_THRESHOLD > + } = WebConsoleUtils; > + > + if (usageCount >= CONSOLE_ENTRY_THRESHOLD) { > + onPaste = null; > + } could we group these into a single assignment ? ```js const { usageCount, CONSOLE_ENTRY_THRESHOLD } = WebConsoleUtils; let onPaste = !hud.isBrowserConsole && usageCount < CONSOLE_ENTRY_THRESOLD ? this.onPaste : null; ``` ::: devtools/client/webconsole/components/App.js:123 (Diff revision 3) > + // Render the entire Console panel. > + return ( > + div({ > + className: "webconsole-output-wrapper", > + ref: node => { > + this.parentNode = node; do we use `parentNode` in other places ? If not, maybe we could simply call this `this.node` ? ::: devtools/client/webconsole/components/App.js:153 (Diff revision 3) > + } > +} > + > +module.exports = connect( > + (state) => ({ > + notifications: state.notifications.notifications nit: Maybe we could follow what we do for other components and use a selector ? ::: devtools/client/webconsole/components/App.js:155 (Diff revision 3) > + (dispatch) => ({ > + dispatch: dispatch, > + }), nit: could be ```js dispatch => ({ dispatch }) ``` ::: devtools/client/webconsole/reducers/notifications.js:23 (Diff revision 3) > + return { > + notifications: undefined, > + }; could this be simply an array ? it feels weird to have state.notifications.notifications ::: devtools/client/webconsole/reducers/notifications.js:48 (Diff revision 3) > +function append(state, action) { > + return appendNotification(state, action); > +} > + > +function remove(state, action) { > + return removeNotificationWithValue(state.notifications, action.value); > +} > + > +function removeAll() { > + return getInitialState(); > +} so, in the case of the jsterm, do we really need to handle multiple notifications ? I feel like we can only have the self-xss message right ? If so, the reducer should only be about setting the message and removing it, and the state could be as simple as `{selfXSSMessageVisible: true|false}`. If we plan to have other messages with the NotificationBox, we would add specific props to reflect those. Thios way, we are more explicit about what we do in the console, and we don't necessarilly reflect the internals of the NotificationBox. So in short, instead of having a `notifications` reducer, we could have a `jsterm` one, with explicit props for messages. It's also a good start for the refactoring since the jsterm reducer will handle other things (e.g. the history). What do you think of this Honza ?
Comment on attachment 8974729 [details] Bug 1425538 - Use NotificationBox as regular React element; clearing the review flag since I have some question we need to think about
Attachment #8974729 - Flags: review?(nchevobbe)
Thanks for the review Nicolas! (In reply to Nicolas Chevobbe [:nchevobbe] from comment #7) > ::: devtools/client/shared/components/NotificationBox.js:247 > (Diff revision 3) > > /** > > * Render the top (highest priority) notification. Only one > > * notification is rendered at a time. > > */ > > render() { > > - let notification = this.state.notifications.first(); > > + const notifications = this.props.notifications || this.state.notifications; > > could this cause some issues ? like, the state is updated, but the component > still has props.notifications, so it does not reflect the new state ? There are two ways how to maintain state for this component now: 1) Use internal state and modify it through component's API (like e.g. `appendNotification`) This was possible when we rendered the component independently in `JSTerm.getNotificationBox` and we kept `this._notificationBox` reference to the rendered element. This way we could call e.g. `this._notificationBox.appendNotification` 2) Use external state (Redux store) and modify it through Redux actions. This is the current situation (what we want) and rendering happens in the new `App` component. Note, that we don't maintain any explicit reference to the element anymore, so there is no way to use component's API. The external state is passed in through props. Both approaches are independent and not supposed to be used at the same time, so we are safe here. As soon as NotificationBox component is only used with external state (#2) in our code base we could remove support for internal state (#1). #1 is currently used for Toolbox notifications. > ::: devtools/client/shared/components/NotificationBox.js:249 > (Diff revision 3) > > + const content = notification ? > > this.renderNotification(notification) : > > null; > > > > - return div({className: "notificationbox"}, > > + return div({ > > + className: "notificationbox", > > + id: this.props.id}, > > content > > ); > > we could directly return null if `notification` is falsy maybe ? If the > content is null, there's no need for us to render an empty div. The empty div is currently used in a test (test_notification_box_01.html), so that one would have to be fixed too. We could do it as a follow up if needed. > ::: devtools/client/shared/components/NotificationBox.js:285 > (Diff revision 3) > > + // Custom image URL is not supported yet. > > + if (image) { > > + throw new Error("Custom image URL is not supported yet"); > > + } > > not related to your patch, but this feels weird to me. Should we simply > remove the image prop ? It's been introduced there to keep compatibility with the previous NotificationBox XUL element. I'll file a follow up for it and we can discuss there (there is another point below). It's currently not used, so might be good candidate for removal. > ::: devtools/client/webconsole/actions/notifications.js:37 > (Diff revision 3) > > + * notification list. > > + */ > > +function removeNotification(value) { > > + return { > > + type: REMOVE_NOTIFICATION, > > + value: value, > > nit: can be replaced with `value` only Yep, done > ::: devtools/client/webconsole/components/App.js:69 > (Diff revision 3) > > + dispatch(actions.appendNotification(SELF_XSS_MSG, > > + "selfxss-notification", null, > > + PriorityLevels.PRIORITY_WARNING_HIGH, null, > > + function(eventType) { > > + // Cleanup function if notification is closed by the user. > > + if (eventType == "removed") { > > + inputField.removeEventListener("keyup", pasteKeyUpHandler); > > + dispatch(actions.removeNotification("selfxss-notification")); > > + } > > + } > > + )); > > nit: this is a bit hard to parse, could we pass an object to the action > instead so the value we pass make more sense to the reader ? I refactored the code a bit (moved the function definition out), it should be better now. > ::: devtools/client/webconsole/components/App.js:74 > (Diff revision 3) > > + dispatch(actions.appendNotification(SELF_XSS_MSG, > > + "selfxss-notification", null, > > + PriorityLevels.PRIORITY_WARNING_HIGH, null, > > + function(eventType) { > > + // Cleanup function if notification is closed by the user. > > + if (eventType == "removed") { > > looking at the NotificationBox code, it looks like that we only execute the > callback with the "removed" eventType. > What do you think of not passing the eventType when calling this callback, > and rename the props in the NotificationBox to something like > `onNotificationRemoved` ? This is the same as before, compatibility with notificationxbox XUL element. The notification is also having support for custom set of buttons, not sure now wherther this can be useful for that. Anyway, I'll put this into the same follow up (support for icon removal + simplifying callback). > ::: devtools/client/webconsole/components/App.js:83 > (Diff revision 3) > > + } > > + )); > > + > > + // Remove notification automatically when the user > > + // types "allows pasting". > > + function pasteKeyUpHandler(event2) { > > nit: i think event2 is not used, we can remove it Done > ::: devtools/client/webconsole/components/App.js:88 > (Diff revision 3) > > + function pasteKeyUpHandler(event2) { > > + let value = inputField.value || inputField.textContent; > > + if (value.includes(SELF_XSS_OK)) { > > + dispatch(actions.removeNotification("selfxss-notification")); > > + inputField.removeEventListener("keyup", pasteKeyUpHandler); > > + WebConsoleUtils.usageCount = WebConsoleUtils.CONSOLE_ENTRY_THRESHOLD; > > So, looking at the code below, it seems like we use this as a way to > indicate if we should add the onPast even listener. > Since it looks like a state to me, I think this should go into the redux > store. Agree, but `WebConsoleUtils.usageCount` is still used by Scratchpad. Another reason why to remove the entire Scrachpad btw. But, we need to wait till we have an alternative - something like "Multiline input line" support in e.g. Console side panel (like e.g. Firebug had). > ::: devtools/client/webconsole/components/App.js:107 > (Diff revision 3) > > + let onPaste = hud.isBrowserConsole ? undefined : this.onPaste; > > + > > + const { > > + usageCount, > > + CONSOLE_ENTRY_THRESHOLD > > + } = WebConsoleUtils; > > + > > + if (usageCount >= CONSOLE_ENTRY_THRESHOLD) { > > + onPaste = null; > > + } > > could we group these into a single assignment ? > ```js > const { > usageCount, > CONSOLE_ENTRY_THRESHOLD > } = WebConsoleUtils; > > let onPaste = !hud.isBrowserConsole && usageCount < CONSOLE_ENTRY_THRESOLD > ? this.onPaste > : null; > ``` Yes, done > ::: devtools/client/webconsole/components/App.js:123 > (Diff revision 3) > > + // Render the entire Console panel. > > + return ( > > + div({ > > + className: "webconsole-output-wrapper", > > + ref: node => { > > + this.parentNode = node; > > do we use `parentNode` in other places ? If not, maybe we could simply call > this `this.node` ? Done, renamed to `this.node` > ::: devtools/client/webconsole/components/App.js:153 > (Diff revision 3) > > + } > > +} > > + > > +module.exports = connect( > > + (state) => ({ > > + notifications: state.notifications.notifications > > nit: Maybe we could follow what we do for other components and use a > selector ? Done > > ::: devtools/client/webconsole/components/App.js:155 > (Diff revision 3) > > + (dispatch) => ({ > > + dispatch: dispatch, > > + }), > > nit: could be > > ```js > dispatch => ({ dispatch }) > ``` I refactored the entire statement as follows: ```js const mapStateToProps = state => ({ notifications: getAllNotifications(state), }); const mapDispatchToProps = dispatch => ({ dispatch, }); ``` Since, WebConsole code base is more using explicit function definition for the `connect mehthod`. I also kept the new line, so `mapStateToProps` and `mapDispatchToProps` looks both unified. > ::: devtools/client/webconsole/reducers/notifications.js:23 > (Diff revision 3) > > + return { > > + notifications: undefined, > > + }; > > could this be simply an array ? it feels weird to have > state.notifications.notifications In order to support both internal & external state (#1 and #2 above) for the NotificationBox component I kept the same structure, so all the API is the same regardless of from where the state comes from. Also, `state.notifications.notifications` is now hidden in the selector, so it won't spread across the code base. > ::: devtools/client/webconsole/reducers/notifications.js:48 > (Diff revision 3) > > +function append(state, action) { > > + return appendNotification(state, action); > > +} > > + > > +function remove(state, action) { > > + return removeNotificationWithValue(state.notifications, action.value); > > +} > > + > > +function removeAll() { > > + return getInitialState(); > > +} > > so, in the case of the jsterm, do we really need to handle multiple > notifications ? I feel like we can only have the self-xss message right ? > > If so, the reducer should only be about setting the message and removing it, > and the state could be as simple as `{selfXSSMessageVisible: true|false}`. So, I picked an array of notifications to have the same state structure and API (as mentioned above). > So in short, instead of having a `notifications` reducer, we could have a > `jsterm` one, with explicit props for messages. It's also a good start for > the refactoring since the jsterm reducer will handle other things (e.g. the > history). > > What do you think of this Honza ? I would vote for keeping the list of notifications in separate reducer and implement the history in another reducer - e.g. history? This would allow as to also keep related selectors and actions in independent modules and follow separation of concerns design principle. It should be also possible to share `notifications` reducer and actions modules together with the `NotificationBox` component, so any other tool (e.g. the Toolbox itself) can use the entire feature out of the box. We could move it into the shared dir at some point when needed. So, in the end we might have the following reducers in Console: - filters - messages - notifications - history - prefs - ui Honza
> So, I picked an array of notifications to have the same state structure and API (as mentioned above). it looks like an implementation detail to me, and in the case of the console, it adds a bit of unnecessary overhead.
oops, hit enter too soon. What I was going to say is that maybe we don't need to use the NotificationBox after all ? Maybe a simple div, with the proper styling and a button to close it is enough ? My point here is that our need are pretty simple, and the redux store should reflect this simplicity. With that being said, I'm fine going further with this patch since it moves some complexity out of the JsTerm.js file, and we can have a follow up in which we decide or not to keep the NotificationBox or not. Maybe there are use cases I don't think of that would justify to use the NotificationBox ?
Comment on attachment 8974729 [details] Bug 1425538 - Use NotificationBox as regular React element; https://reviewboard.mozilla.org/r/243102/#review249926 ::: devtools/client/webconsole/actions/notifications.js:41 (Diff revision 5) > +/** > + * Remove all notifications from JSTerm notification list. > + */ > +function removeAllNotifications() { > + return { > + type: REMOVE_ALL_NOTIFICATIONS > + }; > +} if I read the patch correctly, this action is never used. Maybe we can remove it (as well as the constant and the reducer function for it) ? ::: devtools/client/webconsole/components/JSTerm.js:1314 (Diff revision 5) > return [ > - dom.div({id: "webconsole-notificationbox", key: "notification"}), > dom.div({ I think we can directly return the first element now instead of an array, since we don't have the div for the notificationbox anymore
Attachment #8974729 - Flags: review?(nchevobbe) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #14) > if I read the patch correctly, this action is never used. Maybe we can > remove it (as well as the constant and the reducer function for it) ? True, removed > ::: devtools/client/webconsole/components/JSTerm.js:1314 > (Diff revision 5) > > return [ > > - dom.div({id: "webconsole-notificationbox", key: "notification"}), > > dom.div({ > > I think we can directly return the first element now instead of an array, > since we don't have the div for the notificationbox anymore Good point, done. (In reply to Nicolas Chevobbe [:nchevobbe] from comment #13) > oops, hit enter too soon. > What I was going to say is that maybe we don't need to use the > NotificationBox after all ? Maybe a simple div, with the proper styling and > a button to close it is enough ? I think we'll benefit from sharing components/code even if they might look small. > My point here is that our need are pretty simple, and the redux store should > reflect this simplicity. > With that being said, I'm fine going further with this patch since it moves > some complexity out of the JsTerm.js file, and we can have a follow up in > which we decide or not to keep the NotificationBox or not. > Maybe there are use cases I don't think of that would justify to use the > NotificationBox ? (from our offline chat) let's try to share the component (together with reducer & actions, see also follow ups) since building set of shareable components is what we want and - if we face any complex and difficult decisions later we'll reconsider. Follow ups: Bug 1461675 - NotificationBox: share also reducer and action modules Bug 1461678 - NotificationBox: improvements & simplification Honza
I've also updated some comment so make the code clearer. Waiting for try... Thanks Nicolas! Honza
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8d0caf759080 Use NotificationBox as regular React element; r=nchevobbe
@Nicolas, I made some changes and it deserves another look from you: https://reviewboard.mozilla.org/r/243102/diff/7-8/ 1) The NotificationBox prop validation was wrong (but the `notifications` props was never used before). It expects an array, but uses Immutable object. I commented the validation out and updated bug 1461678 to also remove ImmutableJS. The validation should go back as soon as ImmutableJS is gone. 2) 'onPaste' event handler is always there and its implementation decides whether to show the warning or allow pasting. 3) The failing test is now initializing the selfxss prop to fix the failure (I am a bit puzzled why it wasn't necessary before) Honza
Flags: needinfo?(odvarko)
Attachment #8974729 - Flags: review+ → review?(nchevobbe)
Comment on attachment 8974729 [details] Bug 1425538 - Use NotificationBox as regular React element; https://reviewboard.mozilla.org/r/243102/#review250338 ::: devtools/client/shared/components/NotificationBox.js:48 (Diff revisions 7 - 8) > - notifications: PropTypes.arrayOf(PropTypes.shape({ > + // Use `PropTypes.arrayOf` validation (see below) as soon as > + // ImmutableJS is removed. do we have a bug number for this ? It would be nice to add it here if we do ::: devtools/client/webconsole/components/App.js:157 (Diff revisions 7 - 8) > id: "webconsole-notificationbox", > notifications, > }), > JSTerm({ > hud, > - onPaste, > + onPaste: this.onPaste, it would be nice if we won't have to put the listener at all once we've reached the threshold
Attachment #8974729 - Flags: review?(nchevobbe) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #25) > > + // Use `PropTypes.arrayOf` validation (see below) as soon as > > + // ImmutableJS is removed. > > do we have a bug number for this ? It would be nice to add it here if we do Yes, there is one, comment updated. > > - onPaste, > > + onPaste: this.onPaste, > > it would be nice if we won't have to put the listener at all once we've > reached the threshold Agree, I tried it, but the after component re-rendering (with onPaste == null), the listener was still there and executed. Not sure why React keeps it there, perhaps mount/unmount would be be needed for the entire App component to get rid of the listener (not sure about perf impact). I can file a follow up bug if needed. Thanks Nicolas! Honza
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fec69c30e7ad Use NotificationBox as regular React element; r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/fec69c30e7ad
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Priority: P2 → P1
Whiteboard: [boogaloo-mvp]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: