Closed Bug 1440333 Opened 7 years ago Closed 7 years ago

Add a way to query a PanelView for whether it is shown

Categories

(Firefox :: Toolbars and Customization, enhancement, P1)

Product:
Firefox
Component:
Toolbars and Customization
Version:
59 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1440333 - Part 1 - Remove the "in-transition" attribute.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1440333 - Part 2 - Rename the "current" attribute to "visible".
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1440333 - Part 3 - Raise the ViewShown event after the main view is active.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
The use case is bug 1439472, we need an additional view state in addition to "open" and "visible" that can be used at any point to determine whether a listener for the ViewShown event should be added. This would be similar to what the "current" attribute used to do, but would not require a DOM modification.
Blocks: 1440358
Blocks: 1441015
Comment on attachment 8953497 [details] Bug 1440333 - Part 1 - Remove the "in-transition" attribute. https://reviewboard.mozilla.org/r/222728/#review228992
Attachment #8953497 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8953498 [details] Bug 1440333 - Part 2 - Rename the "current" attribute to "visible". https://reviewboard.mozilla.org/r/222730/#review228994
Attachment #8953498 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8953499 [details] Bug 1440333 - Part 3 - Raise the ViewShown event after the main view is active. https://reviewboard.mozilla.org/r/222732/#review229010 ::: browser/components/customizableui/PanelMultiView.jsm:1149 (Diff revision 3) > this._offscreenViewStack.style.maxHeight = maxHeight + "px"; > } > break; > } > case "popupshown": > + let mainPanelView = this.openViews[0]; Why this rather than `this._mainView` like before?
Attachment #8953499 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #13) > > + let mainPanelView = this.openViews[0]; > > Why this rather than `this._mainView` like before? Less indirection, these are small improvements I'm doing as I see them. Also, there's likely an opportunity to remove the _mainView property once it's only used by the _showMainView method.
Comment on attachment 8953499 [details] Bug 1440333 - Part 3 - Raise the ViewShown event after the main view is active. Did you clear the review because of the question, or was this meant to be r+?
Attachment #8953499 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8953499 [details] Bug 1440333 - Part 3 - Raise the ViewShown event after the main view is active. https://reviewboard.mozilla.org/r/222732/#review229044 ::: browser/components/customizableui/PanelMultiView.jsm:1149 (Diff revision 3) > this._offscreenViewStack.style.maxHeight = maxHeight + "px"; > } > break; > } > case "popupshown": > + let mainPanelView = this.openViews[0]; (In reply to :Paolo Amadini from comment #14) > (In reply to :Gijs from comment #13) > > > + let mainPanelView = this.openViews[0]; > > > > Why this rather than `this._mainView` like before? > > Less indirection, these are small improvements I'm doing as I see them. > > Also, there's likely an opportunity to remove the _mainView property once > it's only used by the _showMainView method. OK, but in this context it looks like a bugfix of sorts, e.g. because `this._mainView` doesn't work because the `mainViewID` attribute isn't set in some edgecases, or something. I think it's confusing to have a getter that literally gets you what you want and not use it. I don't understand how this is an 'improvement' - the code is strictly longer and more confusing now than it was before, and it's not like the existing code was inefficient. For one the code isn't hot, for another `document.getElementById` is optimized by the DOM code anyway. Please leave it be here. If you think the getter is superfluous, remove it in a separate cset.
Attachment #8953499 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #16) > a getter that literally gets you what you want That's not literally true, the getter returns a node and we want a PanelView object. Anyways, it's such a minor change that I'm fine leaving it for a dedicated changeset.
Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/9906b468eb78 Part 1 - Remove the "in-transition" attribute. r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/c705b49c2bae Part 2 - Rename the "current" attribute to "visible". r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/2dad70b2a723 Part 3 - Raise the ViewShown event after the main view is active. r=Gijs
Backout by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3b07c9e515 Backed out 3 changesets for firefox-functional-test failures on test_no_certificate.py on a CLOSED TREE
Backed out for firefox-ui-functional-remote-e10s failures in testing/firefox-ui/tests/functional/security/test_no_certificate.p: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3b07c9e5153154f1b1ac7170bac187a9e02298 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2dad70b2a723e67b45563dd3ac254911dd193439&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=164330383 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=164330383&repo=mozilla-inbound&lineNumber=40358 [task 2018-02-26T13:59:14.524Z] 13:59:14 INFO - 1519653554516 Marionette TRACE 42 -> [0,214,"getContext",{}] [task 2018-02-26T13:59:14.524Z] 13:59:14 INFO - 1519653554516 Marionette TRACE 42 <- [1,214,null,{"value":"chrome"}] [task 2018-02-26T13:59:14.524Z] 13:59:14 INFO - 1519653554516 Marionette TRACE 42 -> [0,215,"setContext",{"value":"content"}] [task 2018-02-26T13:59:14.526Z] 13:59:14 INFO - 1519653554518 Marionette TRACE 42 <- [1,215,null,{}] [task 2018-02-26T13:59:14.526Z] 13:59:14 INFO - 1519653554520 Marionette TRACE 42 -> [0,216,"getPageSource",{}] [task 2018-02-26T13:59:14.531Z] 13:59:14 INFO - 1519653554528 Marionette TRACE 42 <- [1,216,null,{"value":"<html dir=\"ltr\" lang=\"en\"><head>\n <title>Mozilla</title>\n <link rel=\"shortcut icon\" type= ... better for everyone.\n <a href=\"mozilla_mission.html\">More</a>\n </p>\n </div>\n\n\n</body></html>"}] [task 2018-02-26T13:59:14.532Z] 13:59:14 INFO - 1519653554528 Marionette TRACE 42 -> [0,217,"setContext",{"value":"chrome"}] [task 2018-02-26T13:59:14.532Z] 13:59:14 INFO - 1519653554528 Marionette TRACE 42 <- [1,217,null,{}] [task 2018-02-26T13:59:14.549Z] 13:59:14 INFO - TEST-UNEXPECTED-ERROR | testing/firefox-ui/tests/functional/security/test_no_certificate.py TestNoCertificate.test_no_certificate | TimeoutException: Timed out after 5.0 seconds
Flags: needinfo?(paolo.mozmail)
I looked at the log of "testing/firefox-ui/tests" and it seems that a Firefox peer review is enough to land changes there, at least for trivial changes, so I guess there is no special step blocking this bug, correct me if I'm wrong. The module situation here is unclear, it doesn't look like firefox-ui tests should prevent Firefox front-end code from landing, which is what it would seem from a superficial reading of <https://wiki.mozilla.org/Modules/All>.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Paolo Amadini from comment #28) > I looked at the log of "testing/firefox-ui/tests" and it seems that a > Firefox peer review is enough to land changes there, at least for trivial > changes, so I guess there is no special step blocking this bug, correct me > if I'm wrong. I think you're right for something this trivial, rs=me for the trivial bugfix. > The module situation here is unclear, it doesn't look like firefox-ui tests > should prevent Firefox front-end code from landing, which is what it would > seem from a superficial reading of <https://wiki.mozilla.org/Modules/All>. Uh, the module system has nothing to do with preventing code from landing. Some of the orange tests are tier-1 (a lot are tier-2). Anything tier-1 going orange gets you backed out.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #29) > > The module situation here is unclear, it doesn't look like firefox-ui tests > > should prevent Firefox front-end code from landing, which is what it would > > seem from a superficial reading of <https://wiki.mozilla.org/Modules/All>. > > Uh, the module system has nothing to do with preventing code from landing. > Some of the orange tests are tier-1 (a lot are tier-2). Anything tier-1 > going orange gets you backed out. What I'm trying to understand is whether a non-trivial change to Firefox that required changing some Firefox UI tests, but not the test infrastructure, would require a review from one of the Firefox UI peers, which is a set of people that is 30 times smaller than the number of Firefox peers, or whether a Firefox peer review would suffice also in that case.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: -- → P1
(In reply to :Paolo Amadini from comment #30) > (In reply to :Gijs from comment #29) > > > The module situation here is unclear, it doesn't look like firefox-ui tests > > > should prevent Firefox front-end code from landing, which is what it would > > > seem from a superficial reading of <https://wiki.mozilla.org/Modules/All>. > > > > Uh, the module system has nothing to do with preventing code from landing. > > Some of the orange tests are tier-1 (a lot are tier-2). Anything tier-1 > > going orange gets you backed out. > > What I'm trying to understand is whether a non-trivial change to Firefox > that required changing some Firefox UI tests, but not the test > infrastructure, would require a review from one of the Firefox UI peers, > which is a set of people that is 30 times smaller than the number of Firefox > peers, or whether a Firefox peer review would suffice also in that case. If you made a non-trivial change to the Fx UI tests, my understanding is that you'd need to ask for review from its peers, yes. In this instance, I don't think you need that. It's not clear to me the individual tests rather than harness code are even covered separately as a module, though. Ask :whimboo.
Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/1ce794bcebdb Part 1 - Remove the "in-transition" attribute. r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/547bb5af0355 Part 2 - Rename the "current" attribute to "visible". r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ebd5155195 Part 3 - Raise the ViewShown event after the main view is active. r=Gijs
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: