Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Y.WidgetModality causes page jumps #1636

Closed
wants to merge 1 commit into from

Conversation

andrewnicols
Copy link
Contributor

It seems that when you create a new Panel which is:

  • modal; and
  • rendered

then the window position jumps to the top of the document.

The default setting for visible is true.
Specifying a visible value of false, and calling show() separately after the dialogue has rendered works around this issue.

This seems to stem from this line of Widget Modality.

As I understand it, the visible parameter is defaulted to true but the widget isn't yet actually visible. As a result, when the focus call is made, and the dialogue isn't yet visible, the browser jumps to the top of the document

I think that this stems from how, while there is a difference between the 'render' and 'rendered' attributes, there is no comparison for 'visible'.

'visible' sometimes records the desired state, and at other times it records the current state.

Example of breakage at http://sandpit.andrewrn.co.uk/yui-broken-panel.html

@andrewnicols
Copy link
Contributor Author

@juandopazo I think that this is your neck of the code

@andrewnicols
Copy link
Contributor Author

Just done a bit more diagnosis, and I'm seeing more issues than I first though.

To prove it to myself, I defined a new module which exteded only:

  • Y.WidgetStdMod; and
  • Y.WidgetModailty

but could not replicate the issue. I even copied all of the JS from Panel but still couldn't, so I took to modifying Y.Panel directly and doign the same thing - still able to reproduce. When I shifted the module, the CSS warnings make me think about the CSS.

Removing bits of the CSS and re-shifting led me to one line which seems to cause it to break consistently:
https://github.com/yui/yui3/blob/master/src/panel/assets/skins/sam/panel-skin.css#L5

Yes, that line does read:

border: 1px solid black;

Removing it and re-shifting makes this work as expected very time.

Then adding back the various components to Panel, I see more breakages. At the moment, I've only added WidgetButtons, but that's enough to break it all over again.

I've also seen it just by having any bodyContent, headerContent, or footerContent present, regardless of the initial visible setting, so I'm going to bed.

@andrewnicols
Copy link
Contributor Author

So looking into this, I think that my original hunch was correct and the border thing I mentioned was the raving of a tired looney.

Thinking about this more, the only time that I see this issue arising is where:

  • modal is true;
  • visible is true;
  • render is true; but
  • rendered is false.

It only happens when the dialogue hasn't yet been rendered.

The pull request I've just submitted for this adds a check on if (this.get('rendered')) before the focus occurs because we simply cannot focus on the dialogue if it isn't both rendered and visible.

This seems to resolve the issue for all cases and permutations that I've tried so far.

I still feel that there should be a distinction between the current state, and the desired state for the 'visible' attribute, but that's going to break backwards compatibility for a lot of people and will definitely require more thought.

@ericf
Copy link
Member

ericf commented Feb 14, 2014

Makes sense to only focus if the widget is rendered.

@clarle
Copy link
Collaborator

clarle commented Feb 14, 2014

Merged into dev-3.x, thanks again for the contribution @andrewnicols!

@clarle clarle closed this Feb 14, 2014
andrewnicols added a commit to andrewnicols/moodle that referenced this pull request Feb 14, 2014
…n jump

This is courtesty of the upstream pull request yui/yui3#1636
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants