Move Autoscroll Icon Out of the DOM

VERIFIED FIXED in Firefox 3 alpha6

Status

()

Firefox
General
VERIFIED FIXED
14 years ago
10 years ago

People

(Reporter: Robert Parenton, Assigned: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com])

Tracking

(Depends on: 3 bugs, Blocks: 2 bugs)

Trunk
Firefox 3 alpha6
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0 -
blocking-firefox3 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want P4])

Attachments

(5 attachments, 9 obsolete attachments)

3.29 KB, image/png
Details
19.26 KB, patch
Details | Diff | Splinter Review
21.60 KB, patch
mano
: review+
Details | Diff | Splinter Review
2.74 KB, patch
mano
: review-
Details | Diff | Splinter Review
42.99 KB, image/png
Details
(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

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

Comment 2

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

Updated

14 years ago
Depends on: 204278
(Reporter)

Updated

14 years ago
Summary: Move Autoscroll Out of the DOM → Move Autoscroll Icon Out of the DOM
(Reporter)

Updated

14 years ago
Blocks: 242466
(Reporter)

Updated

14 years ago
Blocks: 215762
(Reporter)

Updated

14 years ago
Blocks: 238923
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

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

Comment 5

13 years ago
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.

Updated

13 years ago
Flags: blocking-aviary1.0?
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.
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.

Updated

13 years ago
Blocks: 247260

Comment 8

13 years ago
(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.
No longer depends on: 204278
(Reporter)

Updated

13 years ago
Blocks: 213726

Comment 9

13 years ago
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.
Flags: blocking-aviary1.0? → blocking-aviary1.0-
How about just axing the icon?

Comment 11

13 years ago
(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.
(Reporter)

Comment 12

13 years ago
(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 .
*sigh*

Updated

13 years ago
Blocks: 258841

Comment 14

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

Updated

13 years ago
Blocks: 260782

Updated

13 years ago
Blocks: 262928
(Reporter)

Updated

13 years ago
No longer blocks: 262928
(Reporter)

Updated

13 years ago
Blocks: 269798

Comment 15

13 years ago
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

12 years ago
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

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

Comment 18

12 years ago
(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.
(Reporter)

Comment 19

12 years ago
Stop spamming the bug
Depends on: 130078
(Reporter)

Comment 20

12 years ago
Now that we have canvas, would it be possible to implement the autoscroll icon
there?
No.  And this actually does depend on the bug I marked as a dependency.
Assignee: firefox → nobody
QA Contact: general
Version: unspecified → Trunk

Updated

12 years ago
Whiteboard: [sg:want P4]
*** Bug 332735 has been marked as a duplicate of this bug. ***

Comment 23

12 years ago
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"); ...};

Updated

11 years ago
Blocks: 338639
Blocks: 347014

Comment 24

11 years ago
I also happens with _some_ fixed position elements when scrolling
Created attachment 257109 [details] [diff] [review]
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.
Assignee: nobody → cst
Status: NEW → ASSIGNED
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?)
Depends on: 70798
Comment on attachment 257109 [details] [diff] [review]
work in progress, backport from SeaMonkey

See comment 25 and comment 26.
Attachment #257109 - Flags: review?(enndeakin)
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.
Attachment #257109 - Flags: review?(enndeakin)
Comment on attachment 257109 [details] [diff] [review]
work in progress, backport from SeaMonkey

beltzner, see comment 26
Attachment #257109 - Flags: ui-review?(beltzner)
(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.
Created attachment 258603 [details]
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.
Attachment #258603 - Flags: ui-review?(beltzner)
Attachment #258603 - Attachment is patch: false
Attachment #258603 - Attachment mime type: text/plain → image/png

Updated

11 years ago
Attachment #258603 - Attachment is patch: true
Attachment #258603 - Attachment mime type: image/png → text/plain
Attachment #258603 - Attachment is patch: false
Attachment #258603 - Attachment mime type: text/plain → image/png
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 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.
Attachment #258603 - Flags: ui-review?(beltzner) → ui-review+
Attachment #257109 - Flags: ui-review?(beltzner) → ui-review+
Created attachment 258881 [details] [diff] [review]
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.
Attachment #257109 - Attachment is obsolete: true
Created attachment 258922 [details] [diff] [review]
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.
Attachment #258881 - Attachment is obsolete: true
Attachment #258922 - Flags: review?(enndeakin)
No longer depends on: 130078
Attachment #258922 - Flags: review?(enndeakin) → review?(mano)
Created attachment 258930 [details] [diff] [review]
patch v2

Addressed feedback from Neil [Rashbrook].
Attachment #258922 - Attachment is obsolete: true
Attachment #258930 - Flags: review?(mano)
Attachment #258922 - Flags: review?(mano)
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.
Attachment #258930 - Flags: review?(mano) → review-
Created attachment 261464 [details]
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

11 years ago
(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.)

Updated

11 years ago
Flags: blocking-firefox3?

Comment 40

11 years ago
Created attachment 262400 [details] [diff] [review]
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.
Assignee: cst → ventnor.bugzilla
Attachment #262400 - Flags: review?(mano)

Comment 41

11 years ago
Created attachment 262405 [details] [diff] [review]
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.
Attachment #262400 - Attachment is obsolete: true
Attachment #262405 - Flags: review?(mano)
Attachment #262400 - Flags: review?(mano)
going to block on this since there's a big set of deps waiting on a fix.
No longer blocks: 269798, 338639
Flags: blocking-firefox3? → blocking-firefox3+
(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?
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?
> 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?
Assignee: ventnor.bugzilla → cst
Status: ASSIGNED → NEW
Attachment #262405 - Flags: review?(mano)

Comment 46

11 years ago
(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.
Created attachment 263138 [details] [diff] [review]
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.
Attachment #258930 - Attachment is obsolete: true
Attachment #263138 - Flags: review?(mano)

Updated

11 years ago
Attachment #262405 - Attachment is obsolete: true

Comment 48

11 years ago
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

11 years ago
Ah, never mind, I'm blind. You did add that listener but in a different part of the patch.
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.
Attachment #263138 - Flags: review?(mano)
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

10 years ago
(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. :)
(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

10 years ago
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.)
Er, what do you mean by different windows versions? AFAICT the patch here only checks the OS, not its version.

Comment 56

10 years ago
(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

Updated

10 years ago
Target Milestone: --- → Firefox 3 alpha6

Comment 57

10 years ago
Created attachment 266984 [details] [diff] [review]
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 :)
Attachment #266984 - Flags: review?(mano)

Comment 58

10 years ago
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?
I will review this patch tonight. That said, please file the regression either way and cc roc...
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 on attachment 266984 [details] [diff] [review]
Patch 4

Likely my last comments here ;)
Attachment #266984 - Flags: review?(mano)

Comment 62

10 years ago
Created attachment 268210 [details] [diff] [review]
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 :)
Attachment #266984 - Attachment is obsolete: true
Attachment #268210 - Flags: review?(mano)
Comment on attachment 268210 [details] [diff] [review]
Patch 5

let's do this.
Attachment #268210 - Flags: review?(mano) → review+
Chris said he would check it in for you.
Checked in.  Hopefully I got that right.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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.
Is Windows supposed to be using the fugly square autoscroll icon now? Because it is...

Comment 68

10 years ago
(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

10 years ago
Created attachment 268463 [details] [diff] [review]
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.
Attachment #268463 - Flags: review?(mano)

Comment 70

10 years ago
Created attachment 268464 [details] [diff] [review]
Fix square on Windows 2

Bah, missed a small typo.
Attachment #268463 - Attachment is obsolete: true
Attachment #268464 - Flags: review?(mano)
Attachment #268463 - Flags: review?(mano)
Checking for the absence of the icon in the DOM would be good.
Flags: in-testsuite?
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

10 years ago
(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.

Updated

10 years ago
Duplicate of this bug: 22775

Comment 75

10 years ago
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).
Depends on: 304563

Updated

10 years ago
Blocks: 384575

Comment 76

10 years ago
(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.
(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 :-)
Any reason you're not using an ifdef in browser.xml instead of a run-time check?
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).
Attachment #268464 - Flags: review?(mano) → review-
(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.
(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

10 years ago
Created attachment 268621 [details]
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)

Updated

10 years ago
Depends on: 384683

Updated

10 years ago
Attachment #258603 - Attachment is obsolete: true

Comment 83

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

Updated

10 years ago
Blocks: 384646
(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

10 years ago
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.
(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

10 years ago
(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.
(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

Updated

10 years ago
Blocks: 384185
(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.
Status: RESOLVED → VERIFIED

Updated

10 years ago
Depends on: 385034

Comment 90

10 years ago
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

10 years ago
It's not translucent in Minefield 20070618 either. It's not a problem for me though - scrolling draw attention away from the icon.
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

10 years ago
(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).

Updated

10 years ago
Depends on: 385884

Updated

10 years ago
No longer depends on: 385884

Updated

10 years ago
Depends on: 386949

Updated

10 years ago
Depends on: 387018

Updated

10 years ago
Depends on: 387521
Before this change, the mouse cursor used to change when hovering over the autoscroll icon. Was it removed on purpose?
(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).

Updated

10 years ago
Blocks: 368029

Updated

10 years ago
Blocks: 282109

Updated

10 years ago
Depends on: 429305
You need to log in before you can comment on or make changes to this bug.