panels JEP concerns and proposed changes

[Myk Melez]

Hi all,

I've reviewed the Jetpack panels JEP along with the documentation on the XUL panel element and the StickyWin panel implementation for web pages, and I've written up the following list of concerns and proposed changes. Take a look, and let me know your thoughts.

-myk

Bug 130078

My first and most serious concern is a note in the XUL panel documentation that mouse interaction with pages loaded into content iframes in XUL panels doesn't work correctly due to bug 130078.

My initial thinking regarding implementation of the proposal was to load jetpack panel content in a content iframe inside a specially-styled XUL panel, and if that's not possible, then I'm not sure how to implement the proposal.

Perhaps we could insert a content iframe into the browser window and then position it using absolute positioning, although that wouldn't allow us to specify its frame. Maybe nested content iframes would work for that, though, with the outer iframe creating the frame and the inner one loading the panel content.

Constructor Function

The JEP proposes jetpack.panel.open() as the method call for creating a panel, but I think we're better off making it a jetpack.Panel constructor function, which separates instantiation from display (so a jetpack can create a long-lived panel that it regularly updates but only shows on user request), reduces overlap in the API wrt ways to display a panel (cf. the show method), and is consistent with the jetpack.Menu constructor.

Positioning Properties

The JEP, like the XUL panel implementation, uses a single property to specify both the point in the panel and the point in the element to which the panel is attached (i.e. the "anchor" element) that determine the position of the panel. In XUL, this use of a single property makes positioning panels hard, since it's not obvious which part of the property value represents which element.

The JEP makes this better by requiring authors to prepend "with anchor" to anchor element attachment point, but this solution suffers from verbosity and additional parsing complexity.

The StickyWin web page panel implementation uses two different properties to specify these points, and I think that's the best approach, since it's relatively terse, clearly distinguishes the points, and is relatively simple to parse.

The StickyWin names for these properties are "position" and "edge", and those don't seem right, since they bear no relation to the elements for whom they specify points. I'm not quite sure what the best terminology would be, but since anchor is a nautical term, cleat comes to mind, f.e. used in the property names panelCleat and anchorCleat.

anchorCleat violates the anchor metaphor, however, since anchors don't have cleats. Docks do, but dock already has a meaning in UI terminology, so it might be confusing to reuse it here.

Positioning I18N Issues

The positioning property specifies left and right value names. Those were deprecated in XUL some years back in favor of start and end because elements are typically best positioned on the opposite side in RTL locales than they are in LTR ones. CSS3 adopts these names too in its text module (although it retains left and right).

I suggest we employ start and end here too, emphasizing in the docs that they represent left and right in LTR locales and the opposite in RTL ones.

Control Over Focus

The JEP proposes two properties for controlling focus, takeFocus and restoreFocus. takeFocus enables a jetpack to control whether or not a panel gets focus when it is shown, while restoreFocus lets a jetpack control whether or not the previously focused element gets refocused when a panel is hidden.

takeFocus seems reasonable, as there are valid use cases for both possible values. In particular, panels introduced as the result of user action and expectation (like clicking on a statusbar icon) should get focus, while those that occur as the result of some other event (like a change to some data that is periodically refreshed) should not.

However, we should be careful to guide authors in their use, and it's worth considering whether there's some way to automate this by detecting whether or not a panel is being opened by the user (similar to how Firefox's popup blocker feature detects whether or not a request to open a window was made by the user).

(It's also worth considering whether panels that open as the result of some other event are actually notifications that should be specified via a notifications JEP, since the use case for notifications is different and should be handled separately.)

restoreFocus seems less reasonable, because it can leave the UI in a state in which no element has focus, confounding keyboard-centric users, and because the decision seems more dependent on the element that had focus (which the jetpack knows nothing about) than the jetpack.

I suggest we retain takeFocus, perhaps renaming it to simply focus (or maybe focusOnShow to make it clear that it happens when you show the panel) but remove restoreFocus and have Jetpack make the decision about whether to restore focus and what element to restore it to, with the initial version always restoring focus to the previously focused element.

Alternately, let's remove both properties and write a notifications JEP to handle the use cases for notifying users about events and target panels to just those use cases in which user action opens a panel.

contentDocument and contentWindow

I'm not sure we need both these properties, as each is easily obtainable from the other (contentDocument.defaultView and contentWindow.document). I expect that contentDocument will be the more popular property and the one we should provide, although we can note in the docs how to get its window.

Also, just to confirm, do we really want the XPCNativeWrapper-wrapped version of the document (i.e. the contentDocument property of the XUL iframe element)? I assume so, for security, but don't want to assume something that has security implications.

And either way, why not just call it document? As we aren't providing both wrapped and unwrapped variants of this object, we don't have to distinguish between them, and the one behaves enough like the other. We should, of course, document the differences in behavior, but I don't think the name is the key to that, and especially not the name contentDocument.

Best Practices

We should encourage best practices for authors who use panels, like when to use the focus properties (if we retain them) and how to make panels togglable (i.e. by not moving the UI used to open the panel when it opens, so users can close it again by clicking in the same place).

Closing a Panel

We should carefully consider and document how and when a panel can close because of user action. First thoughts:

For panels that open as a result of user action and are focused when they open, they close when the user presses escape or clicks outside the panel.

For panels that do not open as a result of user action and are not focused when they open, they close when the user focuses them and then presses escape or clicks outside the panel or when the user presses their "close" button, which only appears for these kinds of panels.

Alternately, we could limit this JEP to only panels that open as the result of user action and get focus, then handle the other use case as part of a notifications JEP.

Style

The use cases for the style property aren't clear. Since panels can load any HTML, they can apply arbitrary style to the content they load inside the HTML itself. The panel frame itself is likely to be minimal, so opportunities to style it will also be minimal. And perhaps it shouldn't be styleable, so multiple panels have consistent appearance, helping to reinforce with users its consistent behavior (wrt. focus, closing, etc.) across jetpacks.

Width/Height

If we keep the style property, these properties might best be applied via it. Otherwise, we should keep these properties, but it might be worth adding a way for the content to determine the size of the panel, just as statusbar panels can determine their width.

Specifying Content

I wonder if it's possible to simplify the API for specifying the content you want to load into the panel by making a content setter that is polymorphic: if you pass it a URL, it loads the URL; if you pass it some HTML, it displays that; and if you pass it some text, it parses it as HTML or sniffs it to determine its content type.

Anchor Indicators

XUL panels on Mac have triangular "arrows" that point from the panel to its anchor element. StickyWin panels can acquire these via the StickyWin.PointyTip class. I suggest we make Jetpack panels have these on all OSes.

[Myk Melez]

On 11/23/2009 03:01 PM, Myk Melez wrote:
> Perhaps we could insert a content iframe into the browser window and
> then position it using absolute positioning, although that wouldn't
> allow us to specify its frame. Maybe nested content iframes would work
> for that, though, with the outer iframe creating the frame and the
> inner one loading the panel content.

I tested this hack today, and I had some success, although not enough, I
reckon.

I can position a content iframe at an arbitrary place on top of the
browser window chrome (including web page content loaded into browser
tabs) using absolute positioning. I am can also build a frame around the
content by nesting one content iframe inside another.

The iframe can't extend beyond the borders of the window, however
(unlike with XUL panels).

And worse, because XUL ignores z-index, every time a node is added to
the browser (f.e. when a new tab is opened), it appears on top of the
content iframe. I can fix things by readding the content iframe to the
document at that point, but it then reloads the document loaded into the
content iframe, resetting the panel.

I don't really see a workaround for that at this point, and I don't
think we can ship a panel implementation that resets all panels when a
user opens a new tab, so I suspect we need to get folks from the
platform team to take a look at this and give us some advice.

-myk