search

phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

improve Dialog #433

Open pixelzoom opened 8 years ago

pixelzoom commented 8 years ago

In https://github.com/phetsims/joist/issues/357#issuecomment-248452092, @ariel-phet said:

@samreid and @pixelzoom please discuss and collaborate on improving dialog. We are adding dialogs in several sims with increased functionality so now would be a good time to improve the implementation. How about come up with a plan and lets discuss a developer meeting, and we can prioritize further there.

In https://github.com/phetsims/joist/issues/357#issuecomment-248759332, I said:

@samreid and I discussed the general status of Dialog today via Skype. But we have no plan yet for how to move forward. The first order of business would likely be to triage related issues, evaluate the current implementation, then decide whether to revised the currently implementation or start over.

CM edit, so I don't have to search comments for link to design doc: https://docs.google.com/document/d/19h0U5iqeChirUvVRaudUbmSP_J3yMn7FgnILWrUq9-c/edit

pixelzoom commented 8 years ago

Open issues related to Dialog and its subtypes:

EDIT:

pixelzoom commented 8 years ago

Design decisions that need to be made before proceeding:

User interface: • What is the desired look for dialogs and their subcomponents? • What options should dialogs have and what are the defaults? (close button, title bar, modality,...)

Software design: • Should dialogs and other popups (menus, check box lists,...) share a common framework? • What Repo should contain the dialog implementation?

@ariel-phet Please assign someone to be responsible for the user-interface design decisions.

SR.EDIT: added a software design question

samreid commented 8 years ago

@ariel-phet said:

We are adding dialogs in several sims with increased functionality

If you could elaborate on the increased functionality, it will increase the probability we can implement something that will address the proposed use cases.

samreid commented 8 years ago

In the issue linked above, I asked @ariel-phet to help me prioritize/schedule Dialog work relative to my other work.

samreid commented 8 years ago

In the above issue, @ariel-phet and I decided:

We can start on design meetings, with a design doc. What do we need dialogs for? Getting a "spec" first. May start this week. May take multiple sessions to make sure we have a good plan.

I'll unassign from myself and put on my calendar for Oct 10 to see if it is ready for my attention.

ariel-phet commented 8 years ago

~~Design document started: https://docs.google.com/document/d/12UkhG7lU-dLBCa-A8FcamkrPBLQxr6ft33Igbw6a5U8/edit#~~

CM edit: The design doc was moved to: https://docs.google.com/document/d/19h0U5iqeChirUvVRaudUbmSP_J3yMn7FgnILWrUq9-c/edit

samreid commented 8 years ago

Please assign to me and @pixelzoom when this is ready to move to implementation phase.

ariel-phet commented 8 years ago

Assigning over to @amanda-phet for the moment since she has taken the lead on this project

amanda-phet commented 8 years ago

At the 10/20/16 design meeting I believe we wrapped up all questions regarding this dialog box. The design specs are outlined in the design document.

samreid commented 8 years ago

The next step will be to review the proposed design with @pixelzoom before implementation work can start. I'll put it on-hold until @pixelzoom is back.

ariel-phet commented 8 years ago

@samreid - @pixelzoom is back

samreid commented 8 years ago

Welcome back @pixelzoom! The next step will be to review the proposed design with @pixelzoom before implementation work can start.

samreid commented 8 years ago

@pixelzoom please reassign me after you have reviewed the proposed design and when we are ready to start implementing.

pixelzoom commented 8 years ago

Comments on current design:

Buffer zone for restricting content (option)

The proposed solution (shown with a mockup) seems complicated. There must be a better way.

Text entry

I don't know what this means. PhET HTML5 sims don't support text entry.

Dismiss with tap outside dialog • Option for non-modal dialogs to dismiss by clicking outside

If a dialog is non-modal, it makes no sense to dismiss it when clicking outside.

If one modal dialog pops up another modal dialog? We may need a stack of dialogs.

Yes, modal dialogs should stack.

Mockups show an icon at the bottom of the screen, presumably for moving? I don't think this is needed. And elsewhere it says "Can click anywhere within panel to move".

Missing requirements:

• Resize icon in lower-right corner presents the same "buffer zone" issues as close button. • How is movability of a dialog made accessible? • How are dialogs constrained to the browser window? • When the browser window is resized, are dialogs that would become "offscreen" moved? • Can dialogs overlap the navigation bar, or are they constrained to the ScreenView? (imo, the latter)

Additional options: • whether to dim everything behind a modal dialog • lineWidth (dialog border) • xMargin and yMargin (margins between dialog edge and content) • close button touchArea and mouseArea x/y dilations • resize handle touchArea and mouseArea x/y dilations

samreid commented 8 years ago

@ariel-phet how do you want to address @pixelzoom's questions? Should we schedule another design meeting, or see if a subgroup can answer them over issue comments?

pixelzoom commented 7 years ago

Requirement from #292:

If a Dialog has modal: false, then it had better have hasCloseButton:true. If not, there's no way to close the dialog. Add an assertion to verify this.

pixelzoom commented 7 years ago

In https://github.com/phetsims/joist/issues/315, it was proposed that Dialog should move to sun. This is something to think about when reimplementing Dialog. Does it require things that are specific to joist? Should/can any of those things be moved to sun?

pixelzoom commented 7 years ago

Requirement from #341: Dialog needs options for close button pointerAreas.

Following the convention used elsewhere, the options and default values would be:

closeButtonTouchAreaXDilation: 0,
closeButtonTouchAreaYDilation: 0,
closeButtonMouseAreaXDilation: 0,
closeButtonMouseAreaYDilation: 0,
pixelzoom commented 7 years ago

Fix the buggy placement of the close button, as reported in #346.

ariel-phet commented 7 years ago

Addressing @pixelzoom comments in design document

ariel-phet commented 7 years ago

@amanda-phet I believe we got to all questions in the last design meeting, if there are any stragglers from this issue, perhaps you @pixelzoom and I can hash them out in this issue or over skype or such?

pixelzoom commented 7 years ago

Raising priority and labeling for developer meeting. I've encounter dialog issue in projectile-motion and a11y this week. Dialogs are obviously an important part of the PhET UI toolkit, and there has been little-to-no progress in the 7 months since this issue was opened.

pixelzoom commented 7 years ago

Keeping the "meeting:design" label here too, since it appears that design may have stalled out (see https://github.com/phetsims/joist/issues/359#issuecomment-265519415). @ariel-phet @amanda-phet What needs to be done to finish design?

pixelzoom commented 7 years ago

Looking through the related issues in https://github.com/phetsims/joist/issues/359#issuecomment-249622050.... 1 of 10 issues have been addressed. And the 1 addressed (#289) was addressed by deleting unused code - hardly an improvement to Dialog.

pixelzoom commented 7 years ago

Apparently having a close button on dialogs is important for a11y, see https://github.com/phetsims/joist/issues/413#issuecomment-287875311. Have we considered this during dialog design? If a close button is needed for a11y, is there any reason for a close button to be optional, or at least the default?

pixelzoom commented 7 years ago

4/27/17 dev meeting: @jessegreenberg will pick this up.

ariel-phet commented 7 years ago

@jessegreenberg just a note - I know you are a bit "under the gun" with publishing JT. However, this work would be good to get some momentum on once accessible JT is published.

jessegreenberg commented 7 years ago

@ariel-phet Understood!

jessegreenberg commented 7 years ago

@ariel-phet it looks like the last design meeting generated some design questions that haven't been answered yet.

• How is movability of a dialog made accessible? • How are dialogs constrained to the browser window? • When the browser window is resized, are dialogs that would become "offscreen" moved? • Can dialogs overlap the navigation bar, or are they constrained to the ScreenView? • Separate screens: what if you have a modal dialog open on a screen, can you switch screens?

Regarding

• How is movability of a dialog made accessible?

I will research a bit and ask folks at the IDRC, I am not sure if there is any standard for this.

Regarding

• Can dialogs overlap the navigation bar, or are they constrained to the ScreenView? • Separate screens: what if you have a modal dialog open on a screen, can you switch screens?

There is a response to this in the document:

No, modal dialogs take over the entire sim, not just the screen of the sim. Non-modal dialogs can stay up on a screen.

Why wouldn't we want screen specific modal dialog behavior? Screen specific informational dialogs could be modal within the ScreenView but could persist when switching screens. @jbphet pointed out that the FluorescentCellsPictureDialog in the "Multiple Cells" screen of gene-expression-essentials would ideally behave like a modal dialog relative to the rest of the ScreenView, but persist when switching screens.

jessegreenberg commented 7 years ago

If there are >1 non-modal dialogs, whichever is dragged should pop to the front.

For a11y, if focus enters a Dialog, it should also pop to the front.

For the close button:

Standard blue focus rectangle

It could be, but blue is not the standard focus highlight color for PhET.

EDIT: Ah, blue seems to be the standard color for browsers with a white background.

jessegreenberg commented 7 years ago

Do we want any non-modal Dialogs that will stay visible over multiple screens or will all non-modal dialogs be screen specific?

jessegreenberg commented 7 years ago

I didn't see it in the design doc, but it was mentioned that the implementation should generally support more than Dialogs - pop ups, combo boxes and such could generally leverage this.

jessegreenberg commented 7 years ago

I did some searching and asked the IDRC in a meeting about non-modal dialogs. We found these examples: https://jqueryui.com/dialog/#modal-confirmation http://accessibility.athena-ict.com/aria/examples/bubbledialog.shtml

These two examples don't support dragging or resizing the dialog with the keyboard.

ariel-phet commented 7 years ago

@jessegreenberg please find a time to discuss open questions with @amanda-phet and then we can schedule time at a design meeting as needed.

zepumph commented 7 years ago

Tagging #424 for dev meeting instead, let's keep this parent issue top level.

jessegreenberg commented 7 years ago

https://github.com/phetsims/joist/issues/424 is another issue that will be added to the list of improvements associated with this issue.

zepumph commented 7 years ago

@jessegreenberg, I have been involved in a lot of workarounds for Dialog hide/dispose recently. I hate spending time on code that I know shouldn't be there, and won't stick around. How much time do you have to devote to this issue, because I don't mind helping. I have a little time perhaps this week that could be spent diving into separating the two methods, would that step on your toes?

jessegreenberg commented 7 years ago

@zepumph I will be working on this in the coming weeks. It sounds like most of the issues you had were related to #424, which should be mostly fixed (though is still open for review). Let me know if you run into any other pressing issues.

zepumph commented 7 years ago

I think that was the main one. Thank you for taking the lead on this. Greatly appreciated.

zepumph commented 7 years ago

In Dialog I still see 

listener: function() {

          // This setTimeout call is a workaround until hide and dispose are separated. It should be removed once
          // https://github.com/phetsims/joist/issues/424 is complete.
          setTimeout( function() { self.hide(); }, 0 );
        },
        accessibleFire: function() {

          // the active element must be focused after the Dialog is hidden, so this must also be wrapped in a
          // timeout. This should also be removed once https://github.com/phetsims/joist/issues/424 is complete.
          setTimeout( function() { self.focusActiveElement(); }, 0 );
        },

Do you think these setTimeout calls are ready to be removed?

jessegreenberg commented 7 years ago

Honestly, I am not clear on why they were necessary in the first place, but comments indicate that they can be removed if they were for #424, I will make note in that issue.

zepumph commented 7 years ago

If I remember correctly, hide calling dispose would happen before the closeButton's 'fire' event could "finish." In terms of PhET-iO, this meant that the 'end' emitter for the fire event would never be called, because a child of the fire event, would destroy the component itself. I'm not sure about the accessibility listener.

pixelzoom commented 7 years ago

@ariel-phet FYI, the lack of closure on this issue is resulting in work-around solutions that are being copy-pasted between sims. See phetsims/projectile-motion#118

samreid commented 7 years ago

In https://github.com/phetsims/phet-io/issues/1147#issuecomment-320355721 @zepumph and I realized that we will (probably) be able to delete BarrierRectangle, and instead use Display.addInputListener (see VertexNode or CircuitElementNode for example usage). It is unclear whether Display.addInputListener will gracefully support dialogs that pop up other dialogs (hopefully yes, but it remains to be seen)

jonathanolson commented 7 years ago

Aren't the barrier rectangle(s) there so that if Dialog 1 opens Dialog 2 (opening Dialog 3) that each level (and everything behind it) would be progressively more grayed out?

samreid commented 7 years ago

There is only one BarrierRectangle instance per sim at the moment and when n>=1 in the modal node stack it does not change its look.

jessegreenberg commented 7 years ago

When this work is done, come back to https://github.com/phetsims/joist/issues/208 to check how this impacts size of Sim.js.

pixelzoom commented 6 years ago

FYI, I'm once again running into problems with Dialog -- in developing new game components (https://github.com/phetsims/vegas/issues/59) and in Equality Explorer (https://github.com/phetsims/equality-explorer/issues/48). As far back as 12/2015, I've reported issues with Dialog that are still not addressed. And this issue (redesign) appears to have stalled out.

Assigning to @ariel-phet to review and prioritize.

ariel-phet commented 6 years ago

Communicated with @jessegreenberg and @amanda-phet via slack. Sounds like they are going to check in to see if anything is left on the design side ( @amanda-phet was pretty sure things had been settled previously).

@jessegreenberg said he could work on this project this week. Marked for dev meeting to see if he needs some assistance.

pixelzoom commented 6 years ago

3/29/18 dev meeting: @andrea-phet and I will triage this issue.