242621 - Move Autoscroll Icon Out of the DOM

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Robert Parenton]

We should try to move the autoscroll icon out of the DOM, as it is causing all
sorts of problems.  In Bug 215762, Jonas Sicking proposes moving the icon into
the chrome as it would solve many problems.

Just for a rundown of the bugs putting the autoscroll icon in the DOM has
definitely caused, here is a list of them (there may be more that haven't been
discovered):

Bug 215762
Bug 215825
Bug 238815
Bug 242466

------- Jonas Sicking (IBM) said in Bug 215762 -------

I have no idea how autoscrolling is implemented, but if it adds images to the
document DOM then the prettyprint side of things work as designed. It sounds
really scary to me that we modify the document DOM for autoscrolling, that can
send off all sorts of wierd things in the document (mutation events will be
notified, scripts executing on a timer will see a modified DOM).

Personally i would prefer to see autoscoll insert things into the chrome instead
which would take care of all of the above problems.

Comment 1

[Robert Parenton]

Bug 238923 is also caused by having the autoscroll icon in the DOM.

Comment 2

[Jens Bannmann]

This might depend on fixing bug 204278, which currently prevents us from
displaying anything on top of the browser.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why does this depend on bug 204278? Tooltips happily paint over the browser and
they're not inserted in the webpage DOM. Why couldn't the same technique be used
for the autoscroll icon?

Comment 4

[Jens Bannmann]

Jonas: AFAIK, tooltips are always 100% opaque. The autoscroll icon is not.

Comment 5

[Thomas Rutter]

Fixing this would fix many problems.

From my point of view an important reason to fix this would be to fix autoscroll
performance.  It is annoying to a user that autoscroll is so much jerkier and
slower than scrolling with the scroll bar when they're essentially doing the
same thing.  If fixing this would require losing the translucent autoscroll
icon, then I would not be against losing the translucent icon.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, opaqueness is a problem. However it is one that needs to be overcome in
order for a proper autoscroll. Having the icon as part of the webpage (as now)
or stacked on top of the <browser> (as suggested in some bug) is not a good
solution. It would limit the icon to be compleatly inside the browser window,
which it shouldn't be if I click off to the far right of the window for example.

I can see three solutions:
a) Use a <popup>, a'la tooltips, which is partly transparent. This, I think,
   already works on some OSes, like linux and windows.
b) Use platform specific code to show a secondary icon. This would probably be a
   lot of work so it sounds like a bad idea.
c) Remove the stupid icon alltogether. Just change the pointer to indicate that
   we're in autoscroll mode. The icon isn't adding a lot of value anyway.

We could also combine c) with either a) or b) and just show the icon on
platforms that support transparent windows or where it's easy to show a
secondary icon.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Or 

d) use a square icon and rid ourselvs of the transparency problem.

Again, this could be done on platforms where showing a freefloating round icon
is a problem.

Comment 8

[Thomas Rutter]

(In reply to comment #6)
> Having the icon as part of the webpage (as now)
> or stacked on top of the <browser> (as suggested in some bug) is not a good
> solution.

I agree with this logic.  I would propose then that this bug does not in fact
depend on Bug #242621 as listed, because making the icon a XUL element stacked
on top of the browser window is only one possible solution and is not the best
solution.

Comment 9

[Blake Ross]

Autoscroll is an important issue for users migrating from IE. I understand the
issues involved with having the icon in the DOM. However, we're not going to get
to a different solution for 1.0, and I'm not comfortable just axing this feature
unless someone can provide specific sites that actually break because of our
autoscroll implementation.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How about just axing the icon?

Comment 11

[Jens Bannmann]

(In reply to comment #9)
> ... unless someone can provide specific sites that actually break because of our
> autoscroll implementation.

Well, bug 242466 lists a few sites that let the autoscroll icon break because of
CSS rules applied to it. But I guess you meant it the other way round :-) Bug
242466 really is only a cosmetic issue (although the other bugs depending on
this are not).

I agree that a real fix for this (whatever it is) will have to wait after 1.0.

Comment 12

[Robert Parenton]

(In reply to comment #10)
> How about just axing the icon?

We would lose the changed cursor in the current implementation, so it probably
isn't that simple.  See Bug 213726 comment #1 .

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*sigh*

Comment 14

[Gary van der Merwe]

See https://bugzilla.mozilla.org/show_bug.cgi?id=260782

Comment 15

[Emil Tamas]

Any work in progress for this bug ? It's one of the very few things that I
actually hate at FireFox :(. It's slow, buggy, bad implemented, many sites have
started to make custom autoscroll icons (like SitePoint, they even have a
tutorial for creating a custom autoscroll, it's hilarious and sad :( )

I even tried to fix this myself, by hacking my firefox instalation, but my
knowledge about firefox's internals is very limited atm. The best thing I could
do  was to remove the autoscroll image from the DOM and display resize cursors
instead. That's a partial solution for me and just adds a signifiand speed
improvement because the autoscroll image is not displayed and there's no need to
calculate it's alpha regions. The speed is almost like IE's autoscroll which is
pretty damn speedy.

Comment 16

[Alan{H}]

In some ways, this bug is actually desireable.
It's unknown to me if this bug has ever actually thrown anything off, but it HAS
been put to good use.
You can change the autoscroll icon.
See here: http://www.sitepoint.com/blog-post-view.php?id=225620
And it's been done here: http://alan.pixelsandpages.com
So maybe it could just be left.  (If not, there should be an alternate way to
specify a custom scroll icon.)

Comment 17

[Alan{H}]

On windows xp, tooltips are opaque execpt for the SHADOW.
So tooltips really <i>aren't</i> always opaque.

Comment 18

[Emil Tamas]

(In reply to comment #16)
> In some ways, this bug is actually desireable.
> It's unknown to me if this bug has ever actually thrown anything off, but it HAS
> been put to good use.
> You can change the autoscroll icon.
> See here: http://www.sitepoint.com/blog-post-view.php?id=225620
> And it's been done here: http://alan.pixelsandpages.com
> So maybe it could just be left.  (If not, there should be an alternate way to
> specify a custom scroll icon.)
I really don't think promoting proprietary stuff relying to a nasty bug is
desirable...
Also the autoscroll performance is jerky, many users are complaining about this. 

From what I know, untill now you couldn't display a transparent area below the
window but now Deer Park supports canvas&SVG and transparency areas... 

I wonder if a better autoscroll can be implemented using the new addons (like a
transparent window .. completely out of the dom).
The bug also crashes the prettyprint feature from firefox.

Comment 19

[Robert Parenton]

Stop spamming the bug

Comment 20

[Robert Parenton]

Now that we have canvas, would it be possible to implement the autoscroll icon
there?

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No.  And this actually does depend on the bug I marked as a dependency.

Comment 22

[Martijn Wargers (dead)]

*** Bug 332735 has been marked as a duplicate of this bug. ***

Comment 23

[nrlz]

Is it possible to modify the webpage cursor image with CSS and then activate it by changing an attribute on the root node? That way it's not in the DOM, but not in chrome either.

<html autoscroll="true"> ...webpage...
</html>

/* userContent.css */
html[autoscroll] * {cursor:url("data:image/png,..."); !important};

onclick = function() {content.document.setAttribute("autoscroll", "true"); ...};

Comment 24

[felix]

I also happens with _some_ fixed position elements when scrolling

Comment 25

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: work in progress, backport from SeaMonkey

Work in progress backport of SeaMonkey's implementation.
 * I didn't patch pinstripe
 * I haven't tested it thoroughly
 * I may have missed remnants of the in-document implementation
 * Is global.css really where the styling belongs?  I'd think browser.css is better, but it doesn't seem to apply where I stuck the <popup>
 * GTK2 on trunk still doesn't have translucency?  http://lxr.mozilla.org/seamonkey/source/suite/common/utilityOverlay.js#617 SeaMonkey does detection to degrade gracefully on less-featured platforms.  This patch isn't pretty on GTK2.  For SeaMonkey, we have multiple versions of our icon: square, 1-bit transparency, and nicely antialiased w/translucency.
 * I still need to look at further cleanup (I had to make more changes from SeaMonkey's version than I expected - so I did some quick hacks to get it working).
 * The error-accumulation code is not really needed if we stick with the original scrolling speed logic.  SeaMonkey uses an exponential function to allow for lower speeds at the slow end - it involves speeds of less than one pixel per loop iteration.

Comment 26

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

mconnor, beltzner, currently we can have a good icon on Windows.  On Linux, the icon can be round but not anti-aliased (so it'll look worse).  On Mac and other platforms (Be, OS/2), you'd get no transparency at all, so the icon has to be square.  The perf win is significant (the scrolling is much smoother).

So, my question is: should I continue working on cleaning up this patch, or are you not interested?  (Or do you want an #if win32-only version, which might be difficult/ugly?)

Comment 27

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 257109 [details] [diff] [review]
work in progress, backport from SeaMonkey

See comment 25 and comment 26.

Comment 28

[Neil Deakin]

Comment on attachment 257109 [details] [diff] [review]
work in progress, backport from SeaMonkey

Seems OK so far, although I didn't test it.

>+ this._autoScrollPopup.showPopup(document.documentElement,
>+                                            event.screenX,
>+                                            event.screenY,
>+                                            "popup", null, null);
>+            this._ignoreMouseEvents = true;
>+            this._screenX = event.screenX;
>+            this._screenY = event.screenY;
>+            this._startX = event.screenX;
>+            this._startY = event.screenY;

You could just get screenX/Y once and then use the version in this.screenX/Y for the others.

Comment 29

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 257109 [details] [diff] [review]
work in progress, backport from SeaMonkey

beltzner, see comment 26

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #26)
> mconnor, beltzner, currently we can have a good icon on Windows.  On Linux, the
> icon can be round but not anti-aliased (so it'll look worse).  On Mac and other
> platforms (Be, OS/2), you'd get no transparency at all, so the icon has to be
> square.  The perf win is significant (the scrolling is much smoother).

I'd love to see screenshots, but based on what I've read about this bug, it sounds like what we should be doing is pushing this forward on trunk and then trying to clean up any icon messiness that results.

FWIW, I don't think autoscroll is a very common case on Mac. Linux is another story, I guess.

> So, my question is: should I continue working on cleaning up this patch, or are
> you not interested?  (Or do you want an #if win32-only version, which might be
> difficult/ugly?)

I'm interested to see if we can get it to a state where it's not a horrendous regression. Again, screenshots (even mocked up based on the icons we can get) would help me understand the effects.

Comment 31

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: autoscroll icons

Left column: sucky, mac
Middle column: so-so, gtk
Right column: Windows

The zoomed versions are just to show the translucency around the edge of the icon used on Windows.  There's no reason the Windows one can't be translucent in the center (like Firefox has currently) - I guess Neil just didn't like the look and I didn't care enough to change it.  The inner edge of the GTK version could be smoothed, but the outer edge currently can't be.

Comment 32

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 258603 [details]
autoscroll icons

beltzner, keep in mind that the OS supplies a nice soft shadow on Mac.  On Windows when we had the square icon, it also had a nice soft shadow (so it's not as rough as t looks there).

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 258603 [details]
autoscroll icons

Well, I'd love to see these get a little better over time, but this is and OK cost for the benefit.

Also, I do notice that Firefox is, like, the only Mac app with autoscroll. We might just not want to do it at all.

Comment 34

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: almost-complete patch

This is pretty much complete, minus some theme changes I'll handle tomorrow.  I need somebody to create a PNG like SeaMonkey's (see attachment 258603 [details]) but with the Firefox styles (i.e. gray border w/black arrows, and more see-through for the fanciest column).

In SeaMonkey, the autoscroll stuff is global for a window, but for Firefox, stuff in browser.xml isn't shared, so it took a bit more effort to avoid recreating the <popup> for every tab.  I think what I did there is reasonable.

Comment 35

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

The pinstripe CSS could be simplified if pinstripe really can't be used on platforms that have support for better popups.  Somebody needs to create an autoscroll.png like SeaMonkey's.

> You could just get screenX/Y once and then use the version in this.screenX/Y
> for the others.

I think it's clearer that I'm getting the event's coordinates.

I think I broke something in my local tree with the styling of menus somehow.  I don't know how.  I'm hoping it's not related to this patch.

Comment 36

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch v2

Addressed feedback from Neil [Rashbrook].

Comment 37

[Asaf Romano (gone)]

Comment on attachment 258930 [details] [diff] [review]
patch v2

I don't like the global scope usage. I would rather:
1. Make all listeners live in browser.xml.
2. Add the popup element to the <browser> element.
3. If the browser element lives in a tabbrowser, call a corresponding method of tabbrowser in _startScroll and do nothing.
4. In tabbrowsser, create the popup under the tabbrowser element and listen to the events via its nsIDOMEventListener implementation, than call the corresponding methods in the active browser element.
5. Make sure we stop scrolling when switching tabs.

Comment 38

[Dão Gottwald [:dao]]

Attachment: autoscroll icons v2

(In reply to comment #34)
> need somebody to create a PNG like SeaMonkey's (see attachment 258603 [details]) but with
> the Firefox styles (i.e. gray border w/black arrows, and more see-through for
> the fanciest column).

(In reply to comment #31)
> The inner edge of the GTK version
> could be smoothed, but the outer edge currently can't be.

Comment 39

[[e-mail]]

(In reply to comment #37)
>2. Add the popup element to the <browser> element.
That's not actually possible - children of the <browser> element are effectively display: none !important;

>3. If the browser element lives in a tabbrowser, call a corresponding method of
>tabbrowser in _startScroll and do nothing.
>4. In tabbrowsser, create the popup under the tabbrowser element and listen to
>the events via its nsIDOMEventListener implementation, than call the
>corresponding methods in the active browser element.
If you don't want to store the popup in a window global, then one possible workaround would be for tabbrowser to have its own built-in autoscroll popup which it would set as the cached popup in each browser.

>5. Make sure we stop scrolling when switching tabs.
The popup already captures key events (incidentally at the window level), so that you either can't change tabs or stop scrolling when you try, depending on which key combo you try to use. (Mouse capture is handled by the patch.)

Comment 40

[Michael Ventnor]

Attachment: New patch

We can't put content in the browser element, however I have written something that will try to keep as much to itself as possible. It doesn't use the window scope, however for the sake of simplicity it appends the icon into the global document. But, the autoscroll icon gets a "popuphidden" listener which closes every issue that I know of that arises from doing this. Try it.

It implements all the graphics restrictions mentioned in comment 26 and needs Dao's great image from comment 38 (which already has Beltzner's blessings) in the toolkit/themes/*stripe/global/icons folder.

Don't worry Mano, we stop scrolling when switching tabs ;) We also get back the lovely fixed-ness instead of the lag in bug 324819.

Comment 41

[Michael Ventnor]

Attachment: New patch 1.1

Apparently you can middle- and right-click at the same time, and the context menu will push the autoscroll icon downwards. This patch fixes that.

Comment 42

[Mike Connor [:mconnor]]

going to block on this since there's a big set of deps waiting on a fix.

Comment 43

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #42)
> going to block on this since there's a big set of deps waiting on a fix.
> 

What is preferred - my code or Ventnor's?

Comment 44

[Asaf Romano (gone)]

ugh, I hoped Michael contacted you before updating the patch... :(

(In reply to comment #43)
 
> What is preferred - my code or Ventnor's?

So, dunno, hopefully I don't need to review both?

Comment 45

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

> ugh, I hoped Michael contacted you before updating the patch... :(

No, it was an unpleasant surprise on the day when I was going to actually work on addressing your comments.  I'm not going to spend my time on this if it's not going to be useful.

Michael, doesn't your patch create a new popup every time the user middle-clicks?

Comment 46

[Michael Ventnor]

(In reply to comment #45)
> > ugh, I hoped Michael contacted you before updating the patch... :(
> 
> No, it was an unpleasant surprise on the day when I was going to actually work
> on addressing your comments.  I'm not going to spend my time on this if it's
> not going to be useful.
> 
> Michael, doesn't your patch create a new popup every time the user
> middle-clicks?
> 

It probably would've been a better idea to contact you before I hijacked this bug :P
It does, so that content is only outside the browser widget when it needs to be. I can just leave you to work on the remainder of this bug if you want.

Comment 47

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch v3

(In reply to comment #37)
> (From update of attachment 258930 [details] [diff] [review])
> I don't like the global scope usage. I would rather:
> 1. Make all listeners live in browser.xml.

Why?  I'm not sure I understand what you mean, but listening to just the browser rather than the window adds appreciable complexity.

> 2. Add the popup element to the <browser> element.

As Neil explained, that's not possible.  I went with his suggestion of having a shared popup in the tabbrowser; when tabbrowser creates a browser, it sets the browser's popup to the tabbrowser's popup.  If the browser's creator doesn't provide it with a popup, it uses the global scope.

> 3. If the browser element lives in a tabbrowser, call a corresponding method 
> of tabbrowser in _startScroll and do nothing.

I'm not a fan of that.  I'd like autoscroll to work for non-tabbrowser consumers of <browser> (e.g. view source, mail, whatever), so the code can't be removed from browser.xml, and I don't think we should duplicate all of the code in tabbrowser.

> 4. In tabbrowsser, create the popup under the tabbrowser element and listen to
> the events via its nsIDOMEventListener implementation, than call the
> corresponding methods in the active browser element.

Same issue as #3 - we'd be adding a lot of almost-duplicate stuff to tabbrowser.

> 5. Make sure we stop scrolling when switching tabs.

I think it's impossible to switch tabs while autoscrolling with my new implementation.

Comment 48

[Michael Ventnor]

Comment on attachment 263138 [details] [diff] [review]
patch v3

>+ this._autoScrollPopup.showPopup(document.documentElement,
>+                                              event.screenX,
>+                                              event.screenY,
>+                                              "popup", null, null);
>+            this._ignoreMouseEvents = true;
>+            this._startX = event.screenX;
>+            this._startY = event.screenY;
>+            this._screenX = event.screenX;
>+            this._screenY = event.screenY;
>+
>+            window.addEventListener("mousemove", this, true);
>+            window.addEventListener("mousedown", this, true);
>+            window.addEventListener("mouseup", this, true);
>+            window.addEventListener("contextmenu", this, true);

Maybe also add this._autoScrollPopup.addEventListener("popuphidden",...
to stop the scrolling? This will make 100% sure that scrolling never continues after the icon has gone away.

There are more than just mouse events that make popups go away.

Comment 49

[Michael Ventnor]

Ah, never mind, I'm blind. You did add that listener but in a different part of the patch.

Comment 50

[Asaf Romano (gone)]

So:
 * Wouldn't it be cleaner to add popuphidden handling to the browser binding (in handleEvent, that is) and then do:
this._autoScrollPopup.addEventListener("popuphidden", this, true);

Then you don't need to store anything on the global object.

 * Pinstripe is only used on OS X, thus you don't need support for eye-candy images in there.
 * I would %ifdef winstripe's global.css file rather than setting "helper" attributes on the popup element leading to non-cheap style rules set.

Comment 51

[Asaf Romano (gone)]

In SM you would probably prefer to keep separate css files and #ifdef jar.mn, assuming you don't want to pre-process css files either.

Comment 52

[Robert Kaiser]

(In reply to comment #51)
> In SM you would probably prefer to keep separate css files and #ifdef jar.mn,
> assuming you don't want to pre-process css files either.

s/SM/Modern - as the default theme of toolkit-based SeaMonkey (the xpfe one will die on trunk very soon) uses *stripe for everything provided by toolkit/ itself. Oh, and toolkit-based SeaMonkey uses toolkit's <browser>, of course. :)

Comment 53

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #50)
>  * I would %ifdef winstripe's global.css file rather than setting "helper"
> attributes on the popup element leading to non-cheap style rules set.

How would that work?

Comment 54

[Robert Kaiser]

Right, I thought XULRunner should have the same global.css file regardless of what Windows version it is on? (And, BTW, SeaMonkey doesn't need special treatment, it will probably use the same toolkit global.css as other apps on trunk before this patch goes in.)

Comment 55

[Asaf Romano (gone)]

Er, what do you mean by different windows versions? AFAICT the patch here only checks the OS, not its version.

Comment 56

[Michael Ventnor]

(In reply to comment #53)
> (In reply to comment #50)
> >  * I would %ifdef winstripe's global.css file rather than setting "helper"
> > attributes on the popup element leading to non-cheap style rules set.
> 
> How would that work?
> 

You would do 

%ifdef XP_WIN
(Windows-specific CSS)
%endif

and

%ifdef XP_UNIX
(CSS for Unix-based OS's, but since OSX uses Pinstripe this would only be Linux. Does any other Winstripe platform report as Unix-based?)
%endif

Comment 57

[Michael Ventnor]

Attachment: Patch 4

Addresses review comments, and fixes a nasty issue I found on Linux that would've made the icon not appear at all. This keeps Chris's style (instead of using my own code path like I did earlier) so it should be easier for Mano :)

Comment 58

[Michael Ventnor]

Autoscrolling on Linux has become unusable due to the fix method in bug 343430, only fixing this will make autoscrolling tolerable again. Mano, will you be able to review this soon or are you too busy?

Comment 59

[Asaf Romano (gone)]

I will review this patch tonight. That said, please file the regression either way and cc roc...

Comment 60

[Asaf Romano (gone)]

The style rules should live in browser.css (the toolkit ones), not global.css, also s/0px/0/ and s/border: 0/border: none/.

Comment 61

[Asaf Romano (gone)]

Comment on attachment 266984 [details] [diff] [review]
Patch 4

Likely my last comments here ;)

Comment 62

[Michael Ventnor]

Attachment: Patch 5

(In reply to comment #60)
> The style rules should live in browser.css (the toolkit ones), not global.css,

I forgot to mention that I tried this earlier but it doesn't work. Its because the icon is appended into the root document (since the browser widget sends its content to /dev/null as mentioned earlier) so it doesn't receive the rules in browser.css. global.css is the only logical, working place for its rules.

> also s/0px/0/ and s/border: 0/border: none/.

At least I fixed this, though :)

Comment 63

[Asaf Romano (gone)]

Comment on attachment 268210 [details] [diff] [review]
Patch 5

let's do this.

Comment 64

[Asaf Romano (gone)]

Chris said he would check it in for you.

Comment 65

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Checked in.  Hopefully I got that right.

Comment 66

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

By the way, it was a pain in the butt to check in because something was wrong with Michael's patch v5... so I had to apply v4, diff the v4 patch against v5, hand apply the changes, then diff my patch against v5 to verify that I matched everything properly.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

Is Windows supposed to be using the fugly square autoscroll icon now? Because it is...

Comment 68

[Zulith]

(In reply to comment #67)
> Is Windows supposed to be using the fugly square autoscroll icon now? Because
> it is...
> 

Same here. Looks square
Pic: http://i12.tinypic.com/4lzziom.jpg

Comment 69

[Michael Ventnor]

Attachment: Fix square on Windows

The ifdefs are supposed to fix that very issue, my only explanation is that XP_WIN is not defined in this section of the codebase.

This returns to a JS-based setting but it shouldn't have much overhead since its only done once.

Comment 70

[Michael Ventnor]

Attachment: Fix square on Windows 2

Bah, missed a small typo.

Comment 71

[Jeff Walden [:Waldo]]

Checking for the absence of the icon in the DOM would be good.

Comment 72

[AKIHIRO Misaki (a.k.a Kuden)]

fx-win32-tbox-trunk {Build ID: 2007061502} Windows 2000

Checkin mistake?

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/toolkit/themes/winstripe/global&command=DIFF_FRAMESET&file=global.css&rev1=1.11&rev2=1.12&root=/cvsroot

Winstripe's global.css printed '%ifdef XP_WIN', '%else', '%endif'.
Winstripe's autoscroll icon looks square.

Comment 73

[Michael Ventnor]

(In reply to comment #72)
> fx-win32-tbox-trunk {Build ID: 2007061502} Windows 2000
> 
> Checkin mistake?
> 
> Winstripe's global.css printed '%ifdef XP_WIN', '%else', '%endif'.

No, thats actually a statement for the compiler to follow.

> Winstripe's autoscroll icon looks square.

Yeah, so I think the compiler must be getting it wrong. :P The newest patch changes them to CSS+Javascript.

Comment 75

[Robert Kaiser]

Marking a dependency on the SeaMonkey bug this originally has been based upon, so one can find where the code came from (even though it looks vastly different now).

Comment 76

[Steffen Wilberg]

(In reply to comment #69)
> The ifdefs are supposed to fix that very issue, my only explanation is that
> XP_WIN is not defined in this section of the codebase.
The files are not preprocessed unless you add a star ("*") at the start of the respective jar.mn line. But preprocessing theme files is evil anyway since it breaks cross-platform theme compatibility, so the script-based solution is preferable.

Comment 77

[Ryan VanderMeulen [:RyanVM]]

(In reply to comment #70)
> Created an attachment (id=268464) [details]
> Fix square on Windows 2
> 
> Bah, missed a small typo.
> 

Much better, thanks Michael :-)

Comment 78

[:Gavin Sharp [email: [e-mail]]]

Any reason you're not using an ifdef in browser.xml instead of a run-time check?

Comment 79

[Asaf Romano (gone)]

Comment on attachment 268464 [details] [diff] [review]
Fix square on Windows 2

You just need to mark http://lxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/jar.mn#16 as preprocessed (8).

Comment 80

[Asaf Romano (gone)]

(In reply to comment #76)
> (In reply to comment #69)
> > The ifdefs are supposed to fix that very issue, my only explanation is that
> > XP_WIN is not defined in this section of the codebase.
> The files are not preprocessed unless you add a star ("*") at the start of the
> respective jar.mn line. But preprocessing theme files is evil anyway since it
> breaks cross-platform theme compatibility, so the script-based solution is
> preferable.
> 

Winstipe is already pre-processed in few places. The other option is to override this file in gnomestripe, which is an overkill IMO.

Comment 81

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #79)
> (From update of attachment 268464 [details] [diff] [review])
> You just need to mark
> http://lxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/jar.mn#16
> as preprocessed (8).
> 

@@ -15,3 +15,3 @@ classic.jar:
         skin/classic/global/formatting.css
-        skin/classic/global/global.css
+*       skin/classic/global/global.css
         skin/classic/global/globalBindings.xml

Checked in with r=mano over IRC.

Comment 82

[Doug Wright]

Attachment: empty cursor

This has caused a regression in Thunderbird's autoscroll on Windows - the autoscroll cursor is now an empty square.

(commenting here instead of filing a new bug, because I haven't a clue what component to put it in)

Comment 83

[bio_tube]

Firefox has the same problem with custom themes. In the default one, however, the autoscroll cursor is normal.

Comment 84

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #83)
> Firefox has the same problem with custom themes. In the default one, however,
> the autoscroll cursor is normal.
> 

Yes, the autoscroll icon is now theme-controlled as in SeaMonkey.

Comment 85

[bio_tube]

So to finish solving this bug, we need to make it so that the default icon is kept if the theme doesn't specify one. It's either that or rely on theme developers to update theirs. Somehow I like the first option.

Comment 86

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #85)
> So to finish solving this bug, we need to make it so that the default icon is
> kept if the theme doesn't specify one. It's either that or rely on theme
> developers to update theirs. Somehow I like the first option.

I believe there's another bug open to use default CSS/icons when a theme does not provide an override.  This is not the only patch that has required theme updates; there's a reason themes have maximum and minimum compatible versions.

Comment 87

[bio_tube]

(In reply to comment #86)
> I believe there's another bug open to use default CSS/icons when a theme does
> not provide an override.  This is not the only patch that has required theme
> updates; there's a reason themes have maximum and minimum compatible versions.

I know that. I was just throwing out there. We should try to maintain backward compatibility as much as possible, unless doing so will barely impede the user experience and there's a good reason not too.

Comment 88

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #86)
> I believe there's another bug open to use default CSS/icons when a theme does
> not provide an override.  This is not the only patch that has required theme
> updates; there's a reason themes have maximum and minimum compatible versions.

FYI, you might be refering to this: https://bugzilla.mozilla.org/show_bug.cgi?id=305746

Comment 89

[Mike Connor [:mconnor]]

(In reply to comment #87)

> I know that. I was just throwing out there. We should try to maintain backward
> compatibility as much as possible, unless doing so will barely impede the user
> experience and there's a good reason not too.

For themes, we're going to break stuff all over, it is expected that we're going to break old themes in new versions, and I'm okay with that.  Theme authors can keep up, or not, but shouldn't be setting maxversions unless they're going to keep up aggressively.

Comment 90

[Eldo Varghese]

in lin thunderbird latest trunk the autoscroll is a white circle with arrows and a black circle in the middle, not translucent.
commented as per ctho's request.
 

Comment 91

[bio_tube]

It's not translucent in Minefield 20070618 either. It's not a problem for me though - scrolling draw attention away from the icon.

Comment 92

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

I asked poningru to comment (comment 90) because somebody was saying I busted Thunderbird (comment 82).  What poningru described in comment 90 is the correct behavior, so I think I didn't break Thunderbird.

Comment 93

[Doug Wright]

(In reply to comment #92)
> I asked poningru to comment (comment 90) because somebody was saying I busted
> Thunderbird (comment 82).  What poningru described in comment 90 is the correct
> behavior, so I think I didn't break Thunderbird.
> 

Comment 90 says TB on *Linux* is fine. But comment 82 is about TB on *Windows* being broken (which it still is btw).

Comment 94

[Ori Avtalion (salty-horse)]

Before this change, the mouse cursor used to change when hovering over the autoscroll icon. Was it removed on purpose?

Comment 95

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #94)
> Before this change, the mouse cursor used to change when hovering over the
> autoscroll icon. Was it removed on purpose?
> 

Nope.  Should it be fixed before bug 213726?  (I don't see bug 213726 being fixed any time soon, because the relevant people aren't interested enough).

Comment 96

[Dabur Distributor]

Great! This is a very useful article. Thank you very much for this and please update more blogs to help much better.
If you want to become a Dabur Distributorship, Franchise, Retailers, and more. Visit the official website: https://daburdistributor.com