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

Temperature preset dropdown wrapping in Chrome on 1.3.5.r2 #2059

Closed
CapnBry opened this Issue Aug 6, 2017 · 10 comments

Comments

4 participants
@CapnBry
Contributor

CapnBry commented Aug 6, 2017

What were you doing?

Clicking dropdown to select a temperature preset causes the dropdown button to wrap to a new line.

Works fine in Edge and Internet Explorer, just happens in Chrome. Tested at a variety of window widths and font zoom percentages (both below and above 100%). I will also point out that the new +/- rich temperature editor all wraps to two lines in the TouchUI version, although I am not sure that's your porblem.

What did you expect to happen?

Everything to stay on the same line.

What happened instead?

I got a new line!

Did the same happen when running OctoPrint in safe mode?

Did not attempt

Branch & Commit or Version of OctoPrint

OctoPrint 1.3.5r2

Operating System running OctoPrint

Windows 10 64-bit

Printer model & used firmware incl. version

N/A

Browser and Version of Browser, Operating System running Browser

Chrome 60 64-bit.

Link to octoprint.log

N/A

Link to contents of terminal tab or serial.log

N/A

Link to contents of Javascript console in the browser

The item causing it is the dropdown-backdrop div being inserted by bootstrap between the blue buttons. This div is added because bootstrap wants to properly allow the user to click outside the dropdown to close it on iOS and is display: block so Chrome says "Oh here is a good place to word wrap". This can be avoided by adding css .dropdown-backdrop { display: inline-block; } however I am not sure if it will break the functionality on iOS devices. The real issue is that when the dropdown arrow is clicked it changes from 8px left right padding to 12px left right padding which makes Chrome think it is too wide to fit in the td. I'm sure adjusting the css width percent of temperature_target could also achieve a workable solution.

Screenshot(s)/video(s) showing the problem:

Imgur

I have read the FAQ.

@ntoff

This comment has been minimized.

Show comment
Hide comment
@ntoff

ntoff Aug 6, 2017

Contributor

I don't see that weird wrapping issue in Chrome Version 60.0.3112.90. Try disabling all your 3rd party plugins, I know at least one that messes up the new temperature dropdowns. Re-enable them one by one until you find the one responsible.

noproblemo

Contributor

ntoff commented Aug 6, 2017

I don't see that weird wrapping issue in Chrome Version 60.0.3112.90. Try disabling all your 3rd party plugins, I know at least one that messes up the new temperature dropdowns. Re-enable them one by one until you find the one responsible.

noproblemo

@CapnBry

This comment has been minimized.

Show comment
Hide comment
@CapnBry

CapnBry Aug 6, 2017

Contributor

@well that is very interesting. Note that the dropdown arrow does not change size at all when you click it. The CSS rule that makes it have a left/right margin of 8px is .btn-group>.btn+.dropdown-toggle. The problem is that once I click it, bootstrap inserts a .dropdown-backdrop div between the button and the dropdown-toggle which makes this rule no longer apply so it just gets the 12px left/right padding of the standard .btn class.

I disabled all my plugins in both Chrome and OctoPrint and had the same results. Knowing that the div is only added if touch support is found in the browser, I went into chrome://flags and changed "Touch Events API" from Automatic (the default) to Off. My dropdown then works as yours does, so it seems that your Automatic is off and my Automatic is on. Windows 10 reports "Touch support with 8 touch points" in the system properties, so I am guessing that is what Chrome is going off of.

I guess the other option is to fix the selector to be .btn-group>.dropdown-toggle which will still apply the proper padding. However, there is also another selector .btn-group>.btn+.btn { margin-left: -1px } which also does not match, extends the button by one pixel by changing its margin back to 0px and it wraps again.

So it seems the CSS will need to be re-evaluated for all dropdowns (is this the only one?) to make sure they don't rely on using the + selector because bootstrap.dropdown will insert a div before the dropdown toggle that will break the selector on any system where touch support is present.

Contributor

CapnBry commented Aug 6, 2017

@well that is very interesting. Note that the dropdown arrow does not change size at all when you click it. The CSS rule that makes it have a left/right margin of 8px is .btn-group>.btn+.dropdown-toggle. The problem is that once I click it, bootstrap inserts a .dropdown-backdrop div between the button and the dropdown-toggle which makes this rule no longer apply so it just gets the 12px left/right padding of the standard .btn class.

I disabled all my plugins in both Chrome and OctoPrint and had the same results. Knowing that the div is only added if touch support is found in the browser, I went into chrome://flags and changed "Touch Events API" from Automatic (the default) to Off. My dropdown then works as yours does, so it seems that your Automatic is off and my Automatic is on. Windows 10 reports "Touch support with 8 touch points" in the system properties, so I am guessing that is what Chrome is going off of.

I guess the other option is to fix the selector to be .btn-group>.dropdown-toggle which will still apply the proper padding. However, there is also another selector .btn-group>.btn+.btn { margin-left: -1px } which also does not match, extends the button by one pixel by changing its margin back to 0px and it wraps again.

So it seems the CSS will need to be re-evaluated for all dropdowns (is this the only one?) to make sure they don't rely on using the + selector because bootstrap.dropdown will insert a div before the dropdown toggle that will break the selector on any system where touch support is present.

@ntoff

This comment has been minimized.

Show comment
Hide comment
@ntoff

ntoff Aug 7, 2017

Contributor

Ahhh, yeah I have no touch capability on my PC (also windows 10).

I just tried on my android tablet with chrome and yep, same issue you have, the dropdown wraps. I guess it's a touch display thing. Enabling touch events in chrome on the PC also then makes it wrap.

Contributor

ntoff commented Aug 7, 2017

Ahhh, yeah I have no touch capability on my PC (also windows 10).

I just tried on my android tablet with chrome and yep, same issue you have, the dropdown wraps. I guess it's a touch display thing. Enabling touch events in chrome on the PC also then makes it wrap.

@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Aug 21, 2017

Owner

This sounds like no fun at all because those styles are from bootstrap's CSS and catching all cases might turn ugly. Ew.

Owner

foosel commented Aug 21, 2017

This sounds like no fun at all because those styles are from bootstrap's CSS and catching all cases might turn ugly. Ew.

@CapnBry

This comment has been minimized.

Show comment
Hide comment
@CapnBry

CapnBry Aug 21, 2017

Contributor

Yeah (sad face emoji) I did some quick selectors and found tons of other things that would get included (over 400 in the case of .btn-group>.dropdown-toggle) so that's a non-starter. This is fixed in the latest bootstrap by changing the .dropdown-backdrop to be added after the dropdown element (switch the insertBefore to insertAfter). The class also gets its own new less to force it to be 0px:

.dropdown-backdrop {
  position: fixed;
  left: 0;
  right: 0;
  bottom: 0;
  top: 0;
  z-index: (@zindex-dropdown - 10);
}

I don't know if there are any other "patches" added to the static js/less but that could fix it. EDIT: Just this change fixes it, although I am not sure if it still works properly on iOS.

diff --git a/src/octoprint/static/js/lib/bootstrap/bootstrap.js b/src/octoprint/static/js/lib/bootstrap/bootstrap.js
index 5ca3cc9..9f223c8 100644
--- a/src/octoprint/static/js/lib/bootstrap/bootstrap.js
+++ b/src/octoprint/static/js/lib/bootstrap/bootstrap.js
@@ -687,7 +687,7 @@
       if (!isActive) {
         if ('ontouchstart' in document.documentElement) {
           // if mobile we we use a backdrop because click events don't delegate
-          $('<div class="dropdown-backdrop"/>').insertBefore($(this)).on('click', clearMenus)
+          $('<div class="dropdown-backdrop"/>').insertAfter($(this)).on('click', clearMenus)
         }
         $parent.toggleClass('open')
       }
Contributor

CapnBry commented Aug 21, 2017

Yeah (sad face emoji) I did some quick selectors and found tons of other things that would get included (over 400 in the case of .btn-group>.dropdown-toggle) so that's a non-starter. This is fixed in the latest bootstrap by changing the .dropdown-backdrop to be added after the dropdown element (switch the insertBefore to insertAfter). The class also gets its own new less to force it to be 0px:

.dropdown-backdrop {
  position: fixed;
  left: 0;
  right: 0;
  bottom: 0;
  top: 0;
  z-index: (@zindex-dropdown - 10);
}

I don't know if there are any other "patches" added to the static js/less but that could fix it. EDIT: Just this change fixes it, although I am not sure if it still works properly on iOS.

diff --git a/src/octoprint/static/js/lib/bootstrap/bootstrap.js b/src/octoprint/static/js/lib/bootstrap/bootstrap.js
index 5ca3cc9..9f223c8 100644
--- a/src/octoprint/static/js/lib/bootstrap/bootstrap.js
+++ b/src/octoprint/static/js/lib/bootstrap/bootstrap.js
@@ -687,7 +687,7 @@
       if (!isActive) {
         if ('ontouchstart' in document.documentElement) {
           // if mobile we we use a backdrop because click events don't delegate
-          $('<div class="dropdown-backdrop"/>').insertBefore($(this)).on('click', clearMenus)
+          $('<div class="dropdown-backdrop"/>').insertAfter($(this)).on('click', clearMenus)
         }
         $parent.toggleClass('open')
       }

foosel added a commit that referenced this issue Aug 25, 2017

Fix wrapping of temperature controls on touch devices
Successfully tested in Chrome (desktop & Android), Firefox (desktop &
Android), Safari (desktop & iOS).

Closes #2059
@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Aug 25, 2017

Owner

@CapnBry thanks mate, that patch indeed also fixed it here and doesn't appear to have any negative consequences across browsers (tested a bunch of them, also on mobile).

Just pushed it to staging/maintenance, will go into 1.3.5rc3 (which I'll need to push out anyhow thanks to #2090 and another Windows-only bug I found yesterday) and then merged to maintenance and devel ASAP as well.

Owner

foosel commented Aug 25, 2017

@CapnBry thanks mate, that patch indeed also fixed it here and doesn't appear to have any negative consequences across browsers (tested a bunch of them, also on mobile).

Just pushed it to staging/maintenance, will go into 1.3.5rc3 (which I'll need to push out anyhow thanks to #2090 and another Windows-only bug I found yesterday) and then merged to maintenance and devel ASAP as well.

@foosel foosel added this to the 1.3.5 milestone Aug 25, 2017

@BillyBlaze

This comment has been minimized.

Show comment
Hide comment
@BillyBlaze

BillyBlaze Aug 25, 2017

Collaborator

FYI; This touch event feature detection by bootstrap will work on mobile, however looking at the feature detection, it will not work for IE touch.

Here is a list of why its bad to use this (taken from Modernizr):

  • Modern IE touch devices implement the Pointer Events API instead
  • Some browsers & OS setups may enable touch APIs when no touchscreen is connected
  • Future browsers may implement other event models for touch interactions
  • Older touchscreen devices only emulate mouse events
Collaborator

BillyBlaze commented Aug 25, 2017

FYI; This touch event feature detection by bootstrap will work on mobile, however looking at the feature detection, it will not work for IE touch.

Here is a list of why its bad to use this (taken from Modernizr):

  • Modern IE touch devices implement the Pointer Events API instead
  • Some browsers & OS setups may enable touch APIs when no touchscreen is connected
  • Future browsers may implement other event models for touch interactions
  • Older touchscreen devices only emulate mouse events
@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Aug 25, 2017

Owner

@BillyBlaze Just to clarify, are you saying patching this like I did above is a bad idea, or just that Bootstrap 2 is broken in regards of touch detection in general? Sorry, a bit slow right now ;)

Owner

foosel commented Aug 25, 2017

@BillyBlaze Just to clarify, are you saying patching this like I did above is a bad idea, or just that Bootstrap 2 is broken in regards of touch detection in general? Sorry, a bit slow right now ;)

@BillyBlaze

This comment has been minimized.

Show comment
Hide comment
@BillyBlaze

BillyBlaze Aug 25, 2017

Collaborator

The patch is good, so its about the latter one.

Collaborator

BillyBlaze commented Aug 25, 2017

The patch is good, so its about the latter one.

@CapnBry

This comment has been minimized.

Show comment
Hide comment
@CapnBry

CapnBry Aug 25, 2017

Contributor

Agreed. This is a bit of a hack in bootstrap to begin with and it keying off the document having ontouchstart is widely broad for its intended purpose. For the purposes of this ticket though, I've switched 1.3.5rc3 and it is resolved. I'd also not seen any other weirdness since I changed my local source 4 days ago so I am going to close this if that's ok.

Contributor

CapnBry commented Aug 25, 2017

Agreed. This is a bit of a hack in bootstrap to begin with and it keying off the document having ontouchstart is widely broad for its intended purpose. For the purposes of this ticket though, I've switched 1.3.5rc3 and it is resolved. I'd also not seen any other weirdness since I changed my local source 4 days ago so I am going to close this if that's ok.

@CapnBry CapnBry closed this Aug 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment