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

Fix layout of main toolbar #3671

Merged
merged 1 commit into from Sep 26, 2013
Merged

Conversation

Rob--W
Copy link
Member

@Rob--W Rob--W commented Sep 11, 2013

Before this commit there were two main issues:

  • In small windows, the zoom controls visually floated above the page number
  • In small windows, the (transparent) zoom container covered the go-to-page
    input box, which prevented one from using the input field to quickly
    navigate to a different page.

@Snuffleupagus
Copy link
Collaborator

Also reduced the specifity of the CSS selectors targetting .outerCenter, because I did not find any reason for using specifity 0,1,3,1 over 0,0,2,1. It only added unnecessary complexity of maintaining the style sheet.

The reason was that we wanted to have a slightly different behaviour depending on whether the sidebar was open or not.

Generally I think these changes look good though, with one exception: The zoom controls now "snaps" to the left-hand side at a much larger window width, compared with the current CSS. (Because you removed the selectors.)

Before this commit there were two main issues:

- In small windows, the zoom controls visually floated above the page number
  (e.g. 733px).
- In small windows, the (transparent) zoom container covered the go-to-page
  input box, which prevented one from using the input field to quickly navigate
  to a different page.
@Rob--W
Copy link
Member Author

Rob--W commented Sep 11, 2013

@Snuffleupagus I see. I have reverted all of my original changes, and submitted a new patch. The initially reported problems were solved by replacing 185px with 205px (otherwise the minus-button would float on the page number) and using pointer-events (to allow the user to click through the invisible wrapper).

@Snuffleupagus
Copy link
Collaborator

/botio-windows preview

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.22.172.223:8877/1c358b0bbaf719b/output.txt

@pdfjsbot
Copy link

@Snuffleupagus
Copy link
Collaborator

I've noticed that some browsers (IE) doesn't support pointer events, see: http://caniuse.com/#search=pointer-events.
Couldn't we instead solve this by using z-index? I think something like this would work:

#toolbarViewerLeft {
  z-index: 1;
}
.outerCenter {
  z-index: 0;
}

@Rob--W
Copy link
Member Author

Rob--W commented Sep 23, 2013

@Snuffleupagus The caniuse you've linked to is about (device) pointer events, not about the pointer-events CSS property. pointer-events for HTML works in IE 11.

Your z-index proposal also works, until #toolbarViewerLeft overlaps with .innerCenter (that is a child of outerCenter). When the same situation occurs with pointer-events, the minus/plus buttons are clickable at least.

@Snuffleupagus
Copy link
Collaborator

pointer-events for HTML works in IE 11.

Yes, but it will be an issue with the older versions (9 and 10). I don't know if it really matters though, I just wanted to ask the question. Let's wait for the official opinion.

@Snuffleupagus
Copy link
Collaborator

This PR will also fix: https://bugzilla.mozilla.org/show_bug.cgi?id=913747.

Snuffleupagus added a commit that referenced this pull request Sep 26, 2013
@Snuffleupagus Snuffleupagus merged commit 9fcb1a9 into mozilla:master Sep 26, 2013
@Snuffleupagus
Copy link
Collaborator

Thanks for the patch!

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.

None yet

3 participants