Closed Bug 242621 Opened 21 years ago Closed 17 years ago

Move Autoscroll Icon Out of the DOM

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3 alpha6

People

(Reporter: robert, Assigned: csthomas)

References

(Depends on 3 open bugs)

Details

(Whiteboard: [sg:want P4])

Attachments

(5 files, 9 obsolete files)

(deleted), image/png
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
asaf
: review+
Details | Diff | Splinter Review
(deleted), patch
asaf
: review-
Details | Diff | Splinter Review
(deleted), image/png
Details
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.
Bug 238923 is also caused by having the autoscroll icon in the DOM.
This might depend on fixing bug 204278, which currently prevents us from displaying anything on top of the browser.
Depends on: 204278
Summary: Move Autoscroll Out of the DOM → Move Autoscroll Icon Out of the DOM
Blocks: 242466
Blocks: 215762
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?
Jonas: AFAIK, tooltips are always 100% opaque. The autoscroll icon is not.
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.
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.
Blocks: 247260
(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
Blocks: 213726
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-
(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.
(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 .
Blocks: 258841
Blocks: 260782
Blocks: 262928
No longer blocks: 262928
Blocks: 269798
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.
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.)
On windows xp, tooltips are opaque execpt for the SHADOW. So tooltips really <i>aren't</i> always opaque.
(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.
Stop spamming the bug
Depends on: 130078
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
Whiteboard: [sg:want P4]
*** Bug 332735 has been marked as a duplicate of this bug. ***
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"); ...};
Blocks: 338639
I also happens with _some_ fixed position elements when scrolling
Attached patch work in progress, backport from SeaMonkey (obsolete) (deleted) — Splinter Review
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?)
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.
Attached image autoscroll icons (obsolete) (deleted) —
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 - Attachment is patch: false
Attachment #258603 - Attachment mime type: text/plain → image/png
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+
Attached patch almost-complete patch (obsolete) (deleted) — Splinter Review
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
Attached patch patch (obsolete) (deleted) — Splinter Review
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)
Attachment #258922 - Flags: review?(enndeakin) → review?(mano)
Attached patch patch v2 (obsolete) (deleted) — Splinter Review
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-
Attached image autoscroll icons v2 (deleted) —
(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.
(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.)
Flags: blocking-firefox3?
Attached patch New patch (obsolete) (deleted) — Splinter Review
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)
Attached patch New patch 1.1 (obsolete) (deleted) — Splinter Review
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)
(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.
Attached patch patch v3 (deleted) — Splinter Review
(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)
Attachment #262405 - Attachment is obsolete: true
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.
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.
(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?
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.
(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
Target Milestone: --- → Firefox 3 alpha6
Attached patch Patch 4 (obsolete) (deleted) — Splinter Review
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)
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)
Attached patch Patch 5 (deleted) — Splinter Review
(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
Closed: 17 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...
(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
Attached patch Fix square on Windows (obsolete) (deleted) — Splinter Review
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)
Attached patch Fix square on Windows 2 (deleted) — Splinter Review
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.
(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.
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
Blocks: 384575
(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.
Attached image empty cursor (deleted) —
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)
Depends on: 384683
Attachment #258603 - Attachment is obsolete: true
Firefox has the same problem with custom themes. In the default one, however, the autoscroll cursor is normal.
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.
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.
(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
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
Depends on: 385034
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.
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.
(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).
Depends on: 385884
No longer depends on: 385884
Depends on: 386949
Depends on: 387018
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).
Blocks: 368029
Blocks: 282109
Depends on: 429305
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: