Closed
Bug 1235350
Opened 8 years ago
Closed 8 years ago
Storage Inspector needs a simplified inline editor
Categories
(DevTools :: Storage Inspector, defect)
DevTools
Storage Inspector
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: miker, Assigned: miker)
References
Details
Attachments
(1 file, 2 obsolete files)
Unfortunately our current inline editor causes a world of problems when we make any attempt to use it in the Storage Inspector. This is because we are not really using a table in the document but a bunch of hboxes and labels. After trying to make it work for a few days we have turned out creating a new, simpler widget. the widget will be based upon the following API: initEditableFields({ triggerEvent: "dblclick", selector: "div.someClass", or an array of CSS Selectors onShow: Function(), onHide: Function(), onChange: Function(newValue), ownerDocument: Document }); We will only have two events (no matter how many inline editors are in the document). These events will be attached to the document. This means that to add all of the inline editors to the storage inspectors cookie table we would use: initEditableFields({ ownerDocument: this.document, selector: "[data-type=cookies] .table-widget-cell", onChange: this.onChange }); The onChange event is passed the new value and the node in the event of a change. It is very simple to then notify the backend of any changes.
Assignee | ||
Comment 1•8 years ago
|
||
devtools/client/shared/widgets/TableWidget.js ============================================= * Set a datatype for each table in order to make it possible to access a table's actor. * onChange emits EVENTS.CELL_EDIT along with row and change info. * onMousedown closes any editors if the area outside the table is clicked. * makeFieldsEditable is called by ui.js when the appropriate actor has a getEditableFields method e.g. getEditableFields("cookie", ["isExpired", "value", ...]) * Remove stuff using the destructor. * Stop focusing the cell as it always focused the row instead often making the clicked cell jump outside the visible area. * Prevent errors in onClick and onMousedown listeners. * The actual EditableFieldsEngine widget. We decided to keep this inside TableWidget.js as is currently only used by tableWidget.js. devtools/client/storage/ui.js ============================= * In destroy() we remove event listeners and call table.destroy (the table's destructor was not called previous to adding this). * When we receive EVENTS.CELL_EDIT we call the appropriate actor's cellEdit() method passing in the row, columnid and change data. We don't need this for all datatypes but this makes this method very generic. * Switched dates from .toLocaleString() to .toUTCString() as this is what is used with all storage types in the browser. devtools/client/themes/widgets.css ================================== * Make non-editable cells look readonly in dark and light themes. Tests will follow with the cookie frontend patch. Review commit: https://reviewboard.mozilla.org/r/35249/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35249/
Attachment #8720268 -
Flags: review?(pbrosset)
Assignee | ||
Comment 2•8 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eeca70ce1de5
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8720268 [details] MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35249/diff/1-2/
Attachment #8720268 -
Attachment description: MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?=pbrosset → MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?pbrosset
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8720268 [details] MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35249/diff/2-3/
Assignee | ||
Comment 5•8 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=01908955b1f5
Assignee | ||
Updated•8 years ago
|
Attachment #8720268 -
Flags: review?(pbrosset)
Comment 6•8 years ago
|
||
https://reviewboard.mozilla.org/r/35249/#review32075 ::: devtools/client/shared/widgets/TableWidget.js:121 (Diff revision 3) > + return this._parent.getAttribute("data-type") || ""; Or maybe `return this._parent.dataset.type || "";` ::: devtools/client/shared/widgets/TableWidget.js:125 (Diff revision 3) > + this._parent.setAttribute("data-type", newType); Or maybe `this._parent.dataset.type = newType;` ::: devtools/client/shared/widgets/TableWidget.js:183 (Diff revision 3) > + if (nodeName === "label" || nodeName === "textbox") { Does this cover the whole table? The comment above suggests that we only want to do this if the click happened outside the table, maybe a better way would be to test if event.target is inside `this._parent`? `this._parent.contains(event.target)` ::: devtools/client/shared/widgets/TableWidget.js:200 (Diff revision 3) > + * @param {Array} editableColumns s/Array/String|Array And change the comment to indicate that both a String and an Array are accepted ::: devtools/client/shared/widgets/TableWidget.js:203 (Diff revision 3) > + makeFieldsEditable: function(type, editableColumns) { I don't understand the need for a `type` here. There's only one datatype per TableWidget instance, right? So why should you need to pass it again here. The table instance knows its own type, so this method shouldn't have to request it again as an argument. Moreover, it looks to me like this type is used solely to craft a selector that will target the right table. If that's the case, couldn't we just get rid of the this type notion altogether, and pass a root element in the `makeFieldsEditable` function: ``` this._editableFieldsEngine = makeFieldsEditable({ root: this._parent, selectors: selectors, inChange: this.onChange }); ``` This way `selectors` could simply be: `selectors.push("#" + id + " .table-widget-cell");` because the inplace editor widget would know which root to search this in. ::: devtools/client/shared/widgets/TableWidget.js:223 (Diff revision 3) > + onChange: this.onChange I suggest removing this onChange callback and instead make the inplace editor widget emit events with the EventEmitter API. This way, you don't restrict these events to be received by only one consumer. ``` this._editableFieldsEngine = makeFieldsEditable({...}); this._editableFieldsEngine.on("change", this.onChange); ``` Also, more consistent with how we deal with events in other parts of devtools. ::: devtools/client/shared/widgets/TableWidget.js:952 (Diff revision 3) > if (event.button == 0 && event.originalTarget == this.header) { You can replace `event.originalTarget` with `target` here since you defined a variable just before. ::: devtools/client/shared/widgets/TableWidget.js:1163 (Diff revision 3) > + ownerDocument._events = {}; What's the reason for storing event information on the ownerDocument here? Couldn't you just store that information on the EditableFieldsEngine instance instead? ::: devtools/client/shared/widgets/TableWidget.js:1164 (Diff revision 3) > + events = ownerDocument._events[triggerEvent] = []; `_events[triggerEvent]` makes me think that this widget potentially supports multiple trigger types, but the `options` object only seem to support one string being passed. Why using `triggerEvent` as a key to the `_events` object? ::: devtools/client/shared/widgets/TableWidget.js:1166 (Diff revision 3) > + this.tbody = ownerDocument.querySelector(".table-widget-body"); If instead of passing the doc in the options, as suggested earlier, we pass the table root element, then we can add the event to the root. And we don't need the widget to know too much about the DOM structure of the table. ::: devtools/client/shared/widgets/TableWidget.js:1204 (Diff revision 3) > + textbox = doc.createElementNS(XUL_NS, "textbox"); > + textbox.id = "inlineEditor"; > + textbox.setAttribute("data-triggerEvent", event.type); > + > + textbox.addEventListener("keydown", this.onKeydown); > + textbox.addEventListener("blur", this.onBlur); > + this.copyStyles(label, textbox); Couldn't we create the input field once and move it around the table as needed, instead of re-creating the DOM node everytime and then removing it in onBlur? We wouldn't have to add/remove the event listeners everytime either this way. ::: devtools/client/shared/widgets/TableWidget.js:1250 (Diff revision 3) > + let data = this.getValuesForCurrentRow(label); I know we said that this inline editor would only be used in a table for now, but still, it feels wrong to have it access table-specific data directly. It feels like we should be separating concerns better by not having code in this widget that knows about how the table works internally (i.e. in the `getValuesForCurrentRow` function below). I think all this widget should do is emit the "change" event with a reference to the label that was just edited and the old and new value. Then the table widget would get that event and, using the label reference, would know hich row, column, etc... basically would figure out the rest of the information by itself. ::: devtools/client/shared/widgets/TableWidget.js:1258 (Diff revision 3) > + options.onChange(data); As mentioned before, I'd rather do `this.emit("changed", data);` here. ::: devtools/client/shared/widgets/TableWidget.js:1281 (Diff revision 3) > + let cols = [...doc.querySelectorAll(".table-widget-column")]; Can't a single document have several instances of a TableWidget? If no, it seems odd to have that limitation. ::: devtools/client/shared/widgets/TableWidget.js:1325 (Diff revision 3) > + let column = textbox.parentNode; Same comment as earlier, I'd rather this widget not have to know about TableWidget too much. It should be fairly easy to de-couple the two things here: Have the inplace editor emit an event when it is blurred with (shift-)tab, so that its consumer (the TableWidget in this case) can decide where to move next and focus the previous or next editor. ::: devtools/client/shared/widgets/TableWidget.js:1367 (Diff revision 3) > + let type = textbox.getAttribute("data-triggerEvent"); > + let newEvent = new win.MouseEvent(type); > + cell.dispatchEvent(newEvent); I think it'd be better to add a new method to the inplaceeditor widget, something like: `trigger(label)` So that you can either actually trigger the event to start the edit mode on a label, or programmatically call this method with the reference to the label. Makes it more useful I think. Also this way you can move the tabbing navigation logic to the TableWidget and have it call this method. ::: devtools/client/shared/widgets/TableWidget.js:1424 (Diff revision 3) > + let style = source.ownerGlobal.getComputedStyle(source); TIL: `ownerGlobal`. Does that only work in XUL documents though? Too bad. ::: devtools/client/shared/widgets/TableWidget.js:1438 (Diff revision 3) > + "width" Does the source element always have a computed width in the table? I suppose there's no other width calculation to be done. ::: devtools/client/storage/ui.js:157 (Diff revision 3) > + actor.cellEdit(JSON.stringify(data)); `cellEdit` might not be a great name for the actor method. I understand that the current code is very generic, but maybe this name is pushing it a little bit too far. "Things" that storage actors deal with aren't tables/cols/cells. So maybe `editItem` instead?
Assignee | ||
Updated•8 years ago
|
Attachment #8720268 -
Flags: review?(pbrosset)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8720268 [details] MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35249/diff/3-4/
Assignee | ||
Updated•8 years ago
|
Attachment #8720268 -
Flags: review?(pbrosset)
Comment 8•8 years ago
|
||
Whoops, I had started the review way before you removed the review request and uploaded a new patch. I guess my comments still apply though.
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #8) > Whoops, I had started the review way before you removed the review request > and uploaded a new patch. I guess my comments still apply though. Yup, thanks for the review. Most of your comments are for stuff that enabled a bunch of different editors on a page but we really don't need that... actually this means I can tidy the code a lot. Moving table stuff out is going to be a challenge because it means people will need to craft their our tab navigation etc. but it shouldn't be too bad. I will wait for a decent try run before requesting review again... one test in particular should never have worked as far as I can see but probably failed silently.
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36027/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36027/
Assignee | ||
Updated•8 years ago
|
Attachment #8720268 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8722434 -
Flags: review?(pbrosset)
Comment 11•8 years ago
|
||
Comment on attachment 8722434 [details] MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?pbrosset https://reviewboard.mozilla.org/r/36027/#review33133 I have a few comments below about the code changes. ::: devtools/client/shared/test/browser_tableWidget_basic.js:11 (Diff revision 1) > - "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > - "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + If these aren't needed anymore, they should be removed, not commented out. ::: devtools/client/shared/test/browser_tableWidget_keyboard_interaction.js:11 (Diff revision 1) > - "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > - "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + Ditto ::: devtools/client/shared/test/browser_tableWidget_mouse_interaction.js:11 (Diff revision 1) > - "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > - "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + Ditto ::: devtools/client/shared/test/browser_tableWidget_mouse_interaction.js:174 (Diff revision 1) > - ok(doc.querySelector("[sorted]"), "Although, something else should be sorted on"); > + ok(doc.querySelector("[sorted]"), > + "Although, something else should be sorted on"); nit: this formatting might be better: ``` ok(doc.querySelector("[sorted]"), "Although, something else should be sorted on"); ::: devtools/client/shared/test/browser_tableWidget_mouse_interaction.js:189 (Diff revision 1) > event = Promise.defer(); > - table.menupopup.addEventListener("popupshown", function onPopupShown(e) { > + table.menupopup.addEventListener("popupshown", function onPopupShown() { > table.menupopup.removeEventListener("popupshown", onPopupShown); > event.resolve(); > - }) > + }); > info("right clicking on the first column header"); > node = table.tbody.firstChild.firstChild.firstChild; > click(node, 2); > yield event.promise; The head.js file for this test actually loads shared-head.js which defines the `once` helper function, so you can replace this with: ``` info("right click on the first column header"); node = table.tbody.firstChild.firstChild.firstChild; let onPopupShown = once(table.menupopup, "popupshown"); click(node, 2); yield onPopupShown; ``` ::: devtools/client/shared/test/browser_tableWidget_mouse_interaction.js:195 (Diff revision 1) > node = table.tbody.firstChild.firstChild.firstChild; Oh my god, this looks pretty bad. I know this was here before, so we can change this later. But at least, can you file a bug to clean up tablewidget tests to be less DOM dependent? The `firstChild.firstChild.firstChild` is hard to read, know what it does, and ight break easily when we change the markup. At least this should be done by a helper function in head.js. ::: devtools/client/shared/test/browser_tableWidget_mouse_interaction.js:219 (Diff revision 1) > - table.menupopup.addEventListener("popupshown", function onPopupShown(e) { > + table.menupopup.addEventListener("popupshown", function onPopupShown() { > table.menupopup.removeEventListener("popupshown", onPopupShown); > event.resolve(); > - }) > + }); > info("right clicking on the first column header"); > node = table.tbody.firstChild.firstChild.firstChild; > click(node, 2); > yield event.promise; Same comment about using `once` ::: devtools/client/shared/test/browser_tableWidget_mouse_interaction.js:245 (Diff revision 1) > - table.menupopup.addEventListener("popupshown", function onPopupShown(e) { > + table.menupopup.addEventListener("popupshown", function onPopupShown() { > table.menupopup.removeEventListener("popupshown", onPopupShown); > event.resolve(); > - }) > + }); > info("right clicking on the first column header"); > node = table.tbody.firstChild.firstChild.firstChild; > click(node, 2); > yield event.promise; And here. ::: devtools/client/shared/test/browser_tableWidget_mouse_interaction.js:279 (Diff revision 1) > - table.menupopup.addEventListener("popupshown", function onPopupShown(e) { > + table.menupopup.addEventListener("popupshown", function onPopupShown() { > table.menupopup.removeEventListener("popupshown", onPopupShown); > event.resolve(); > - }) > + }); > info("right clicking on the first column header"); > node = table.tbody.firstChild.firstChild.firstChild; > click(node, 2); > yield event.promise; And here. ::: devtools/client/shared/widgets/TableWidget.js:182 (Diff revision 1) > + this.editBookmark = colName === uniqueId ? change.newValue : row[uniqueId]; What is the intent for `editBookmark`? Can you clarify? I think there should be a comment explaining what this is for, because it's not straightforward. ::: devtools/client/shared/widgets/TableWidget.js:186 (Diff revision 1) > + onTab: function(event) { Please add a jsdoc comment block for this function, explaining that this is intended to be called by the inplace editor when tab/shift-tab is pressed in edit-mode. ::: devtools/client/shared/widgets/TableWidget.js:212 (Diff revision 1) > + let textbox = doc.getElementById("inlineEditor"); Isn't this supposed to be `event.target`? If not, I think the inline editor should pass itself as an argument to `onTab`, rather than having to get it by ID here. ::: devtools/client/shared/widgets/TableWidget.js:214 (Diff revision 1) > + let cols = filterVisible(doc.querySelectorAll(".table-widget-column")); Shouldn't you do `this._parent.querySelectorAll` instead of using `doc` here. What if there are more than 1 table in the same document? ::: devtools/client/shared/widgets/TableWidget.js:255 (Diff revision 1) > + if (this.tbody.querySelector("#inlineEditor")) { Might be more elegant if you could check something like `this._editableFieldsEngine.isEditing` instead. ::: devtools/client/shared/widgets/TableWidget.js:265 (Diff revision 1) > + selectedCell = this.tbody.querySelector(".theme-selected"); > + if (!selectedCell) { > + return; > + } Looks like we need to always do this, no matter which key is pressed. So, better move it above the `switch` statement and remove it from both `case` below. ::: devtools/client/shared/widgets/TableWidget.js:305 (Diff revision 1) > + let elt = this.document.getElementById("inlineEditor"); Similarly to another comment I made earlier, it would be nicer if you could do this instead: `this._editableFieldsEngine.blur()` ::: devtools/client/shared/widgets/TableWidget.js:819 (Diff revision 1) > + if (this.table.editBookmark) { The usage of `editBookmark` here isn't straightforward either. Can clarify (and add a comment) why when a row gets updated we check if there is a bookmark and send a ROW_SELECTED event? ::: devtools/client/shared/widgets/TableWidget.js:1097 (Diff revision 1) > while (target) { > dataid = target.getAttribute("data-id"); > if (dataid) { > break; > } > target = target.parentNode; I think that `target.closest("[data-id]")` would help make this code simpler. ::: devtools/client/shared/widgets/TableWidget.js:1215 (Diff revision 1) > + * root: someNode, > + * onTab: function(event) { ... } > + * triggerEvent: "dblclick", > + * selectors: Array or comma separated string of CSS Selectors It would help to have a short comment for each of these properties that explains what each of them are, if they are optional, and what they are used for. ::: devtools/client/shared/widgets/TableWidget.js:1314 (Diff revision 1) > + * - <tab> apply the value and move the textbox to the next available field. > + * - <shift><tab> apply the value and move the textbox to the previous > + * available field. This part of the comment should be updated to say that tab and shift-tab actually delagates to the consumer's `onTab` callback who needs to decide where focus goes next. ::: devtools/client/shared/widgets/TableWidget.js:1357 (Diff revision 1) > + this.onTrigger({ > + target: target > + }); Or `this.onTrigger({target});`
Attachment #8722434 -
Flags: review?(pbrosset)
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8722434 [details] MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36027/diff/1-2/
Attachment #8722434 -
Attachment description: MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor → MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?pbrosset
Attachment #8722434 -
Flags: review?(pbrosset)
Assignee | ||
Comment 13•8 years ago
|
||
Looking at try results from the parent changeset: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=91eedd43f7cf There seems to be an issue with Windows pushes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2b475d9de77 I am going to have to build a Windows box to look into this.
Comment 14•8 years ago
|
||
Comment on attachment 8722434 [details] MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?pbrosset https://reviewboard.mozilla.org/r/36027/#review34371 Thanks Mike, this works nicely. I found one remaining bug while tabbing though. It doesn't always happen, but let me give you the steps I used so you can investigate: - Open https://bugzilla.mozilla.org/ (I wasn't logged in) - Open the storage inspector and open the "bugzilla.mozilla.org" coookies table - There's a cookie named something like `_gat_UA-...` and its value is a number. - Edit this number to something else, in my case I changed 1 to 2 - Press TAB The number got changed but the row did not flash orange and the next field did not get receive focus. So I don't know why it doesn't work for this particular value, it does work for others. Apart from this, I only have a few remaining comments below. The most important of which is the one related to how we deal with re-entering edit mode after an update was made. I believe this requires a little bit more work. So, just one more round of review, and I think we're good to go! ::: devtools/client/shared/widgets/TableWidget.js:230 (Diff revisions 1 - 2) > + if (textbox.id !== "inlineEditor") { nit: maybe add a property on EditableFieldsEngine.prototype to refer to this ID. EditableFieldsEngine.prototype = { INPUT_ID: "inlineEditor", ... } ::: devtools/client/shared/widgets/TableWidget.js:268 (Diff revisions 1 - 2) > + // The item may have been edited and had to be recreated when the user has What item? The cell or whole row? Not sure how the TableWidget update process works. What happens if the change fails on the server, is the ROW_EDIT event still going to be emitted in that case (maybe that's what's happening with the bug I found earlier). Also, this looks a bit racy to me, you're starting to listen to the event after triggering the change. In practice it works because the listener is added on keydown, while the change happens on blur, and goes through the protocol, etc... Again, I'm not sure how the TableWidget updates its DOM on data changes, but would there be a way to just always start editing the next cell synchronously (as in the case where changePending is false), and later, when/if an update occurs, then do the right thing by re-entering edit mode on the right cell? Could this also somehow solve the delay I was talking about in my first review? ::: devtools/client/shared/widgets/TableWidget.js:1322 (Diff revisions 1 - 2) > + nit: no need for 2 empty lines (eslint no-multiple-empty-lines rule). ::: devtools/client/shared/widgets/TableWidget.js:1427 (Diff revisions 1 - 2) > + target.classList.remove("flash-out"); The flash-out class seems like something the TableWidget should know about only. I don't think the inplace-editor should have to deal with this. `target` is a cell of the table, and if it needs the `flash-out` class removed at some stage, then the table should do it, not the inplace-editor. ::: devtools/client/shared/test/head.js:4 (Diff revision 2) > +/* eslint no-unused-vars: [2, {"vars": "local"}] */ Actually better to use: ``` /* eslint no-unused-vars: [2, {"vars": "local", "args": "none"}] */ ``` Because we've recently changed the no-unused-vars rule in the global .eslintrc to not complain about args. Note about these changes in head.js: none of them seem to be related to the table edit mode, they mostly seem like eslint changes only. So it might be good to move them to a separate patch before landing.
Attachment #8722434 -
Flags: review?(pbrosset)
Assignee | ||
Comment 15•8 years ago
|
||
https://reviewboard.mozilla.org/r/36027/#review34371 I don't have that cookie but by experimenting I have discovered that editing an expired cookie fails unless you are setting the date. I guess our best option when a cookie is expired is to set the date to now + 24h.
Assignee | ||
Comment 16•8 years ago
|
||
https://reviewboard.mozilla.org/r/36027/#review34371 In the end I decided to set the expiry time to 1 minute away for cookies that have expired... this way we can have some level of editability.
Assignee | ||
Comment 17•8 years ago
|
||
https://reviewboard.mozilla.org/r/36027/#review34371 Hehe... in the very end I decided to just add 10 seconds from the current time and edit the value from there. An edit should never take 10 seconds so this should work well. > What item? The cell or whole row? > Not sure how the TableWidget update process works. > What happens if the change fails on the server, is the ROW_EDIT event still going to be emitted in that case (maybe that's what's happening with the bug I found earlier). > Also, this looks a bit racy to me, you're starting to listen to the event after triggering the change. In practice it works because the listener is added on keydown, while the change happens on blur, and goes through the protocol, etc... > > Again, I'm not sure how the TableWidget updates its DOM on data changes, but would there be a way to just always start editing the next cell synchronously (as in the case where changePending is false), and later, when/if an update occurs, then do the right thing by re-entering edit mode on the right cell? > > Could this also somehow solve the delay I was talking about in my first review? > Not sure how the TableWidget update process works. It is pretty optimal actually... the difficulty is that for some fields we need to delete the cookie and recreate it and other times we just edit cookie values. When we change a field that makes it necessary to recreate a cookie (name, domain or path) the name could have changed so we wait for the whole row to update and move to the row containing the uniqueId that we have received (the table can be sorted according to any column). It is not simply a case of editing the next cell synchronously because if the cookie had to be recreated we need to calculate that position differently than if it is not recreated. I guess we could add another list to the fields... something like causesReload. I am also not happy with it as it is so I have been trying different ways to deal with the issue. > The flash-out class seems like something the TableWidget should know about only. I don't think the inplace-editor should have to deal with this. > `target` is a cell of the table, and if it needs the `flash-out` class removed at some stage, then the table should do it, not the inplace-editor. We now use: let onAnimEnd = () => { this.label.classList.remove("flash-out"); this.label.removeEventListener("animationend", onAnimEnd); }; this.label.addEventListener("animationend", onAnimEnd); this.label.classList.add("flash-out"); Much tidier.
Assignee | ||
Comment 18•8 years ago
|
||
https://reviewboard.mozilla.org/r/36027/#review34371 > > Not sure how the TableWidget update process works. > > It is pretty optimal actually... the difficulty is that for some fields we need to delete the cookie and recreate it and other times we just edit cookie values. > > When we change a field that makes it necessary to recreate a cookie (name, domain or path) the name could have changed so we wait for the whole row to update and move to the row containing the uniqueId that we have received (the table can be sorted according to any column). > > It is not simply a case of editing the next cell synchronously because if the cookie had to be recreated we need to calculate that position differently than if it is not recreated. > > I guess we could add another list to the fields... something like causesReload. > > I am also not happy with it as it is so I have been trying different ways to deal with the issue. I have spent long enough improving this. The root of the problem is that we cannot insert the textbox into the correct row until the row actually exists and we know that the row has not been removed. We do not know these things until we have a response from the actor. We cannot work around waiting for an edit result from the actor before moving the textbox because: * All columns can be sorted so changing any value can move us to any row. We don't necessarily know which row without knowing the datatype e.g. dates are in the format `Wed, 09 Mar 2016 15:24:54 GMT` but if you type the wrong day `Mon, 09 Mar 2016 15:24:54 GMT` it will be corrected by the cookie manager. * An edit may result in the deletion of a cookie e.g. changing the domain, name or path... we don't know this until we have the edit result from the actor. * Editing of some values necessitates e.g. deleting and creating a cookie. From the tables perpective this is what happens: * The original cookie is deleted destroying the original row... the input field should not exist at this stage. * A new cookie is created. * The row is created in the appropriate place (could be any row depending on sort order). This is our first signal that the edit was successful and therefore our first chance to show the input field in the appropriate place. We could anticipate the result by adding the possibility to validate the table and type the columns (bug 1251561) but even then we cannot insert the textbox into the correct row until the row actually exists and we know that the row has not been removed. Saying these things, I am unable to produce any race conditions and the UI is reasonably responsive. I believe that switching to a react component would really help here (bug 1201742).
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8722434 [details] MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36027/diff/2-3/
Attachment #8722434 -
Flags: review?(pbrosset)
Assignee | ||
Comment 20•8 years ago
|
||
My comments in comment 18 were really just reasons that making editing stable is difficult. In the end I managed it and it works well... the textbox moves immediately on Tab and if the row moves somewhere as the result of an edit then the textbox moves with it.
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8722434 [details] MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36027/diff/3-4/
Comment 22•8 years ago
|
||
https://reviewboard.mozilla.org/r/36027/#review37695 ::: devtools/client/shared/test/browser_tableWidget_mouse_interaction.js:12 (Diff revision 4) > +"use strict"; > + > const TEST_URI = "data:text/xml;charset=UTF-8,<?xml version='1.0'?>" + > "<?xml-stylesheet href='chrome://global/skin/global.css'?>" + > - "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > - "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + These comments should probably be removed. ::: devtools/client/shared/widgets/TableWidget.js:240 (Diff revision 4) > + * keep this in mind whenever editing this method. > + * > + * @param {Event} event > + * Keydown event > + */ > + onEditorTab: function(event) { Have you tested if it works in conjunction with filtering? ::: devtools/client/shared/widgets/TableWidget.js:272 (Diff revision 4) > + // If the row has been deleted we should bail out. > + if (!uniqueId) { > + return; > + } > + > + // Find the column we need to move to. Seems like we're doing the same kind of thing for row keyboard navigation. Can't we have a common function for those 2 things? Something like "getKeyboardTarget(type, direction, data, current)". type: column/row direction: up/down/right/left data: cell list/row list current: current row/cell The function would take in account filtered rows as well. ::: devtools/client/shared/widgets/TableWidget.js:273 (Diff revision 4) > + if (!uniqueId) { > + return; > + } > + > + // Find the column we need to move to. > + if (event.shiftKey) { I would add a comment to clarify: "If shift is pressed, move backwards". It wasn't obvious for me why there was a special treatment for shift at first. ::: devtools/client/shared/widgets/TableWidget.js:286 (Diff revision 4) > + } > + } else if (currentColIndex === cols.length - 1) { > + let id = cols[0].id; > + columnObj = this.columns.get(id); > + let maxColIndex = columnObj.cellNodes.length - 1; > + if (this.selectedIndex >= maxColIndex) { I would call the variable maxRowIndex, or lastRowIndex. Anyway, I don't see why this would ever happen (a case where the currently selected row index would be greater the last row index). ::: devtools/client/shared/widgets/TableWidget.js:311 (Diff revision 4) > + cells = columnObj.cellNodes; > + cell = cells[this.selectedIndex]; > + editor.edit(cell); > + > + // Remove flash-out class... it won't have been auto-removed because the > + // cell was hidden for editing. I don't understand this part. Even if the cell were hidden for editing, the animationend event should still fire. Or maybe you mean "Avoid flashing cells that just have been edited", in which case keeping the flash is better (it makes sense to flash a row if you change a value). ::: devtools/client/shared/widgets/TableWidget.js:366 (Diff revision 4) > + * Get the cell that will be edited next on tab / shift tab. > + * > + * @param {Event} event > + * Keydown event > + */ > + getEditedCellOnTab: function(event, column) { Seems like this function somewhat repeats some of the algorithm above (onEditorTab), is there a way to share the code ? ::: devtools/client/shared/widgets/TableWidget.js:435 (Diff revision 4) > + event.preventDefault(); > + > + let prevRow = selectedCell.previousSibling; > + if (prevRow.classList.contains("table-widget-column-header")) { > + prevRow = prevRow.parentNode.lastChild; > + } Seems like my code to make sure keyboard navigation excludes cell hidden by filtering was removed. Is that intentional ? ::: devtools/client/shared/widgets/TableWidget.js:443 (Diff revision 4) > + case event.DOM_VK_DOWN: > + event.preventDefault(); > + > + let nextRow = selectedCell.nextSibling || > + selectedCell.parentNode.children[1]; > + this.emit(EVENTS.ROW_SELECTED, nextRow.getAttribute("data-id")); Same question here. ::: devtools/client/shared/widgets/TableWidget.js:483 (Diff revision 4) > + for (let id of editableColumns) { > + selectors.push("#" + id + " .table-widget-cell"); > + } > + > + for (let [name, column] of this.columns) { > + if (editableColumns.indexOf(name) === -1) { nit: if (!editableColumns.includes(name)) ::: devtools/client/storage/ui.js:46 (Diff revision 4) > > +const REASON = { > + NEW_ROW: "new-row", > + NEXT_50_ITEMS: "next-50-items", > + POPULATE: "populate", > + UPDATE: "update" Nice cleanup! Much easier to understand than numbers.
Assignee | ||
Comment 23•8 years ago
|
||
https://reviewboard.mozilla.org/r/36027/#review34371 > I have spent long enough improving this. > > The root of the problem is that we cannot insert the textbox into the correct row until the row actually exists and we know that the row has not been removed. We do not know these things until we have a response from the actor. > > We cannot work around waiting for an edit result from the actor before moving the textbox because: > > * All columns can be sorted so changing any value can move us to any row. We don't necessarily know which row without knowing the datatype e.g. dates are in the format `Wed, 09 Mar 2016 15:24:54 GMT` but if you type the wrong day `Mon, 09 Mar 2016 15:24:54 GMT` it will be corrected by the cookie manager. > * An edit may result in the deletion of a cookie e.g. changing the domain, name or path... we don't know this until we have the edit result from the actor. > * Editing of some values necessitates e.g. deleting and creating a cookie. From the tables perpective this is what happens: > * The original cookie is deleted destroying the original row... the input field should not exist at this stage. > * A new cookie is created. > * The row is created in the appropriate place (could be any row depending on sort order). This is our first signal that the edit was successful and therefore our first chance to show the input field in the appropriate place. > > We could anticipate the result by adding the possibility to validate the table and type the columns (bug 1251561) but even then we cannot insert the textbox into the correct row until the row actually exists and we know that the row has not been removed. > > Saying these things, I am unable to produce any race conditions and the UI is reasonably responsive. I believe that switching to a react component would really help here (bug 1201742). I decided that all the points above were reasons that moving the textbox before committing the previous change made things hard but not impossible. Anyhow, this issue is now fixed.
Comment 24•8 years ago
|
||
I encountered several annoying bugs when playing with this patch: 1. After double-clicking to edit, the columns randomly change their widths. Some values that determine the column layout apparently change. This also happens when switching between different items (types/hosts) in the tree. The column widths should be stable, but aren't. 2. Let there be many storage items, so that the table scrolls. Double-click on some value to edit it. The table will scroll in such a way so that the edited row is out-of-view. The first row visible is the first row AFTER the edited one. 3. Make sure the sidebar with the value details is hidden. Double-click on "Value" to edit. The first click will open the sidebar, causing the "Value" column to move away from under the mouse pointer. The second click will always happen in the just-opened sidebar. My intent to double-click to edit will have failed.
Assignee | ||
Comment 25•8 years ago
|
||
(In reply to Jarda Snajdr [:jsnajdr] from comment #24) > I encountered several annoying bugs when playing with this patch: > > 1. After double-clicking to edit, the columns randomly change their widths. > Some values that determine the column layout apparently change. This also > happens when switching between different items (types/hosts) in the tree. > The column widths should be stable, but aren't. > > 2. Let there be many storage items, so that the table scrolls. Double-click > on some value to edit it. The table will scroll in such a way so that the > edited row is out-of-view. The first row visible is the first row AFTER the > edited one. > > 3. Make sure the sidebar with the value details is hidden. Double-click on > "Value" to edit. The first click will open the sidebar, causing the "Value" > column to move away from under the mouse pointer. The second click will > always happen in the just-opened sidebar. My intent to double-click to edit > will have failed. Item 1 is a known bug. It is caused by us using a XUL table. When dynamic content is injected into a XUL document sometimes the widths and heights of elements are not properly calculated. We plan on switching to a React based HTML table widget in the future to fix this as the XUL bug will probably never be fixed... using XUL we really have the best we can get here. Item 2 is a new on to me... can you log a bug about this and I will fix it? Item 3 is irritating but we are using onClick (to sort columns), onMousedown (to select rows and open the sidebar) and onDoubleclick (to initialize edit mode). I guess we could use settimeout onclick and cancel the event in the case of a doubleclick. Again, could you log a bug as it is probably something that our UX people should look into.
Updated•8 years ago
|
Attachment #8722434 -
Flags: review?(pbrosset)
Comment 26•8 years ago
|
||
Comment on attachment 8722434 [details] MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?pbrosset https://reviewboard.mozilla.org/r/36027/#review36679 Mike's working on a new version of this patch, so I'll stop reviewing now. But since I had made some comments already, I'm posting them here. Maybe they are useful. ::: devtools/client/shared/widgets/TableWidget.js:254 (Diff revisions 2 - 3) > > - for (let selector of this._editableFieldsEngine.selectors) { > - if (col.matches(selector)) { > - return true; > + // Changing any value can change the position of the row depending on which > + // column it is currently sorted on. In addition to this, the table cell may > + // have been edited and had to be recreated when the user has pressed tab or > + // shift+tab. Both of these situations require us to recover our target, > + // select the appropriate row and mode the textbox on to the next cell. s/mode/move ::: devtools/client/shared/test/browser_tableWidget_basic.js:11 (Diff revision 4) > - "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > - "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + I don't remember what we agreed about this change, but if you do need to remove these CSS files because they cause test timeouts, then: - either remove the lines altogether - or add a comment preceding these lines that explains why they need to be commented out ::: devtools/client/shared/test/browser_tableWidget_keyboard_interaction.js:11 (Diff revision 4) > - "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > - "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + Same comment here. ::: devtools/client/shared/test/browser_tableWidget_keyboard_interaction.js:151 (Diff revision 4) > - // pressing up arrow key to select previous row > - node = table.tbody.firstChild.firstChild.children[2]; > - // node should not have selected class > - ok(!node.classList.contains("theme-selected"), > - "Row should not have selected class"); > - info("Pressing up key to select previous row"); > - event = table.once(TableWidget.EVENTS.ROW_SELECTED); > - EventUtils.sendKey("UP", doc.defaultView); > - id = yield event; > + yield testRow("id3", "DOWN", "next row"); > + yield testRow("id4", "DOWN", "next row"); > + yield testRow("id3", "UP", "previous row"); > + yield testRow("id4", "DOWN", "next row"); > + yield testRow("id5", "DOWN", "next row"); > + yield testRow("id6", "DOWN", "next row"); > + yield testRow("id5", "UP", "previous row"); > + yield testRow("id4", "UP", "previous row"); > + yield testRow("id3", "UP", "previous row"); This looks a lot better, thanks ::: devtools/client/shared/test/browser_tableWidget_keyboard_interaction.js:176 (Diff revision 4) > +function* testRow(id, key, destination) { > + let node = getNodeByValue(id); > // node should not have selected class > ok(!node.classList.contains("theme-selected"), > "Row should not have selected class"); > - info("Pressing down key on last row to select first row"); > + info("Pressing " + key + " key to select " + destination); Please use template strings for things like these: info(`Pressing ${key} to select ${destination}`); ::: devtools/client/shared/test/browser_tableWidget_keyboard_interaction.js:180 (Diff revision 4) > id = yield event; > - is(id, "id9", "Correct row was selected after pressing down"); > + > + is(id, id, "Correct row was selected after pressing " + key); Hmm, you're overriding the argument `id` here, and then comparing `id` to `id`, so of course this assertion is always going to pass. ::: devtools/client/shared/test/browser_tableWidget_mouse_interaction.js:11 (Diff revision 4) > - "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > - "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + > + // "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + Same comment about these being commented out. ::: devtools/client/shared/widgets/TableWidget.js:110 (Diff revision 4) > + this.onChange = this.onChange.bind(this); > + this.onEditorDestroyed = this.onEditorDestroyed.bind(this); > + this.onEditorTab = this.onEditorTab.bind(this); > + this.onKeydown = this.onKeydown.bind(this); > + this.onRowRemoved = this.onRowRemoved.bind(this); > + this.document.addEventListener("keydown", this.onKeydown, false); > + this.onMousedown = this.onMousedown.bind(this); > + this.document.addEventListener("mousedown", this.onMousedown, false); I think it's more easily readable if you group all bindings together and do all addEventListeners separately.
Assignee | ||
Comment 27•8 years ago
|
||
https://reviewboard.mozilla.org/r/36027/#review37695 > These comments should probably be removed. Comment updated. // Uncomment these lines to help with visual debugging. When uncommented they // dump a couple of thousand errors in the log (bug 1258285) // "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" + // "<?xml-stylesheet href='chrome://devtools/skin/widgets.css'?>" + > Seems like we're doing the same kind of thing for row keyboard navigation. Can't we have a common function for those 2 things? Something like "getKeyboardTarget(type, direction, data, current)". > type: column/row > direction: up/down/right/left > data: cell list/row list > current: current row/cell > > The function would take in account filtered rows as well. > The function would take in account filtered rows as well. I had that working but those changes must have been lost by a merge error... fixed. > Can't we have a common function for those 2 things? That is how these two methods started out but, although the logic of the functions is similar, the contents of each logical block do very different things and this resulted in a ridiculously overcomplicated, unmaintainable method. getEditedCellOnTab() dumbly moves to the next / previous column and highlights the appropriate row. This makes the interface responsive rather than waiting for EVENTS.ROW_EDIT before adding the textbox. onEditorTab() calls getEditedCellOnTab() so a textbox exists and then acts on the EVENTS.ROW_EDIT event to move the textbox in the event that the row has moved. > I would call the variable maxRowIndex, or lastRowIndex. > > Anyway, I don't see why this would ever happen (a case where the currently selected row index would be greater the last row index). I will switch to ===. Well done spotting the bad variable name... that was a very long day. > I don't understand this part. Even if the cell were hidden for editing, the animationend event should still fire. Or maybe you mean "Avoid flashing cells that just have been edited", in which case keeping the flash is better (it makes sense to flash a row if you change a value). > Even if the cell were hidden for editing, the animationend event should still fire. Nope, we are transitioning on the background color. .flash-out { transition: background .5s; } If this class is added to a cell that is hidden then the animation does not run until the hidden flag is removed. This resulted in each cell flashing when tabbing through the table (when the editor was hidden and the edited cell shown whether the cell had changes or not). > Seems like this function somewhat repeats some of the algorithm above (onEditorTab), is there a way to share the code ? No, as explained above, these methods do sufficiently different things to warrant them being in separate methods. Squeezing it all into one method results in a horrible, unmaintainable glob of code. > Seems like my code to make sure keyboard navigation excludes cell hidden by filtering was removed. Is that intentional ? No, probably lost on rebase. Fixed.
Assignee | ||
Comment 28•8 years ago
|
||
https://reviewboard.mozilla.org/r/36027/#review36679 > Hmm, you're overriding the argument `id` here, and then comparing `id` to `id`, so of course this assertion is always going to pass. I think that was a 3AM thing. > I think it's more easily readable if you group all bindings together and do all addEventListeners separately. And sorted alphabetically.
Assignee | ||
Comment 29•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41719/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41719/
Attachment #8733321 -
Flags: review?(pbrosset)
Assignee | ||
Updated•8 years ago
|
Attachment #8722434 -
Attachment is obsolete: true
Assignee | ||
Comment 30•8 years ago
|
||
Comment on attachment 8733321 [details] MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r+pbro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41719/diff/1-2/
Comment 31•8 years ago
|
||
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #25) > Item 1 is a known bug. It is caused by us using a XUL table. When dynamic > content is injected into a XUL document sometimes the widths and heights of > elements are not properly calculated. We plan on switching to a React based > HTML table widget in the future to fix this as the XUL bug will probably > never be fixed... using XUL we really have the best we can get here. This problem goes away when the table columns' width attributes are specified (e.g., after resizing a column) - then the buggy layout algorithm is probably avoided and the table behaves nicely. The table in Network Monitor specifies widths for all columns in vw units - maybe that would be a good fix also for Storage Inspector, too. The bug would get simply and easily fixed and users wouldn't have to suffer. It would also fix the not-very-nice behavior when switching between tree items in the Storage Inspector, where the column widths change randomly depending on the data in the table. Keeping the column width stable would feel much better. > Item 2 is a new on to me... can you log a bug about this and I will fix it? This is caused by the interaction between the table header elements having "position: sticky" style and the behavior of Element.scrollIntoView(). The edited row is aligned to the top of the scrollable ancestor, but there the row is hidden by the sticky header floating above it. This unfortunate interaction looks almost like a platform bug - position:sticky and Element.scrollIntoView seem to not work together very well. Simple and effective fix would be to not call scrollIntoView at all. When you click on the table cell, it usually means that it's already well visible. The only issue is that it can be partially out-of-view. I checked a few other web apps to see how they handle this situation (Chrome Storage Inspector and Dropbox file rename) and they ignore it. Do you really want to file a separate bug? It's an issue introduced by new code from this bug, and probably should be fixed here, as part of the review process. > > Item 3 is irritating but we are using onClick (to sort columns), onMousedown > (to select rows and open the sidebar) and onDoubleclick (to initialize edit > mode). I guess we could use settimeout onclick and cancel the event in the > case of a doubleclick. Again, could you log a bug as it is probably > something that our UX people should look into. Reported as bug 1258682.
Updated•8 years ago
|
Attachment #8733321 -
Flags: review?(pbrosset) → review+
Comment 32•8 years ago
|
||
Comment on attachment 8733321 [details] MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r+pbro https://reviewboard.mozilla.org/r/41719/#review38427 Thanks for the new patch. All the bugs I have reported so far seem to be fixed. I did notice one remaining bug though, related to filtering. Here's a GIF that hopefully demonstrates it well enough: https://dl.dropboxusercontent.com/u/714210/editmode-filter-bug.gif ::: devtools/client/themes/widgets.css:1189 (Diff revision 2) > + background-color: rgba(255,255,255,0.1); > +} > + > +.theme-light .table-widget-column[readonly] { > + background-color: rgba(0,0,0,0.1); I think these are fine, but it's still worth pinging Helen and have a conversation about whether it makes sense to have a theme variable for greying out readonly things in devtools. I suspect the answer is no, but worth checking.
Assignee | ||
Comment 33•8 years ago
|
||
Comment on attachment 8733321 [details] MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r+pbro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41719/diff/2-3/
Attachment #8733321 -
Attachment description: MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r?pbro → MozReview Request: Bug 1235350 - Storage Inspector needs a simplified inline editor r+pbro
Assignee | ||
Comment 34•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/881863474a3493f5b74fa7bd62f1a8a7e71588d8 Bug 1235350 - Storage Inspector needs a simplified inline editor r+pbro
Comment 35•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/881863474a34
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•