Closed
Bug 242621
Opened 21 years ago
Closed 17 years ago
Move Autoscroll Icon Out of the DOM
Categories
(Firefox :: General, defect)
Firefox
General
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.
Reporter | ||
Comment 1•21 years ago
|
||
Bug 238923 is also caused by having the autoscroll icon in the DOM.
Comment 2•21 years ago
|
||
This might depend on fixing bug 204278, which currently prevents us from
displaying anything on top of the browser.
Reporter | ||
Updated•21 years ago
|
Summary: Move Autoscroll Out of the DOM → Move Autoscroll Icon Out of the DOM
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•20 years ago
|
||
Jonas: AFAIK, tooltips are always 100% opaque. The autoscroll icon is not.
Comment 5•20 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•20 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.
Comment 8•20 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
Comment 9•20 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•20 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•20 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*
Comment 14•20 years ago
|
||
Comment 15•20 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•19 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•19 years ago
|
||
On windows xp, tooltips are opaque execpt for the SHADOW.
So tooltips really <i>aren't</i> always opaque.
Comment 18•19 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•19 years ago
|
||
Stop spamming the bug
Reporter | ||
Comment 20•19 years ago
|
||
Now that we have canvas, would it be possible to implement the autoscroll icon
there?
Comment 21•19 years ago
|
||
No. And this actually does depend on the bug I marked as a dependency.
Updated•19 years ago
|
Assignee: firefox → nobody
QA Contact: general
Version: unspecified → Trunk
Updated•19 years ago
|
Whiteboard: [sg:want P4]
Comment 22•19 years ago
|
||
*** Bug 332735 has been marked as a duplicate of this bug. ***
Comment 23•19 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"); ...};
Comment 24•18 years ago
|
||
I also happens with _some_ fixed position elements when scrolling
Assignee | ||
Comment 25•18 years ago
|
||
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
Assignee | ||
Comment 26•18 years ago
|
||
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?)
Assignee | ||
Comment 27•18 years ago
|
||
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 28•18 years ago
|
||
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)
Assignee | ||
Comment 29•18 years ago
|
||
Comment on attachment 257109 [details] [diff] [review]
work in progress, backport from SeaMonkey
beltzner, see comment 26
Attachment #257109 -
Flags: ui-review?(beltzner)
Comment 30•18 years ago
|
||
(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.
Assignee | ||
Comment 31•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Attachment #258603 -
Flags: ui-review?(beltzner)
Updated•18 years ago
|
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
Assignee | ||
Updated•18 years ago
|
Attachment #258603 -
Attachment is patch: false
Attachment #258603 -
Attachment mime type: text/plain → image/png
Assignee | ||
Comment 32•18 years ago
|
||
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•18 years ago
|
||
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+
Updated•18 years ago
|
Attachment #257109 -
Flags: ui-review?(beltzner) → ui-review+
Assignee | ||
Comment 34•18 years ago
|
||
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
Assignee | ||
Comment 35•18 years ago
|
||
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)
Assignee | ||
Updated•18 years ago
|
Attachment #258922 -
Flags: review?(enndeakin) → review?(mano)
Assignee | ||
Comment 36•18 years ago
|
||
Addressed feedback from Neil [Rashbrook].
Attachment #258922 -
Attachment is obsolete: true
Attachment #258930 -
Flags: review?(mano)
Attachment #258922 -
Flags: review?(mano)
Comment 37•18 years ago
|
||
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-
Comment 38•18 years ago
|
||
(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•18 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•18 years ago
|
Flags: blocking-firefox3?
Comment 40•18 years ago
|
||
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•18 years ago
|
||
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)
Comment 42•18 years ago
|
||
going to block on this since there's a big set of deps waiting on a fix.
Assignee | ||
Comment 43•18 years ago
|
||
(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•18 years ago
|
||
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?
Assignee | ||
Comment 45•18 years ago
|
||
> 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?
Updated•18 years ago
|
Assignee: ventnor.bugzilla → cst
Status: ASSIGNED → NEW
Updated•18 years ago
|
Attachment #262405 -
Flags: review?(mano)
Comment 46•18 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.
Assignee | ||
Comment 47•18 years ago
|
||
(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•18 years ago
|
Attachment #262405 -
Attachment is obsolete: true
Comment 48•18 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•18 years ago
|
||
Ah, never mind, I'm blind. You did add that listener but in a different part of the patch.
Comment 50•18 years ago
|
||
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.
Updated•18 years ago
|
Attachment #263138 -
Flags: review?(mano)
Comment 51•18 years ago
|
||
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•18 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. :)
Assignee | ||
Comment 53•18 years ago
|
||
(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•18 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.)
Comment 55•18 years ago
|
||
Er, what do you mean by different windows versions? AFAICT the patch here only checks the OS, not its version.
Comment 56•18 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•18 years ago
|
Target Milestone: --- → Firefox 3 alpha6
Comment 57•18 years ago
|
||
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•17 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?
Comment 59•17 years ago
|
||
I will review this patch tonight. That said, please file the regression either way and cc roc...
Comment 60•17 years ago
|
||
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•17 years ago
|
||
Comment on attachment 266984 [details] [diff] [review]
Patch 4
Likely my last comments here ;)
Attachment #266984 -
Flags: review?(mano)
Comment 62•17 years ago
|
||
(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 63•17 years ago
|
||
Comment on attachment 268210 [details] [diff] [review]
Patch 5
let's do this.
Attachment #268210 -
Flags: review?(mano) → review+
Comment 64•17 years ago
|
||
Chris said he would check it in for you.
Assignee | ||
Comment 65•17 years ago
|
||
Checked in. Hopefully I got that right.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 66•17 years ago
|
||
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•17 years ago
|
||
Is Windows supposed to be using the fugly square autoscroll icon now? Because it is...
Comment 68•17 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•17 years ago
|
||
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•17 years ago
|
||
Bah, missed a small typo.
Attachment #268463 -
Attachment is obsolete: true
Attachment #268464 -
Flags: review?(mano)
Attachment #268463 -
Flags: review?(mano)
Comment 71•17 years ago
|
||
Checking for the absence of the icon in the DOM would be good.
Flags: in-testsuite?
Comment 72•17 years ago
|
||
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•17 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.
Comment 75•17 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
Comment 76•17 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.
Comment 77•17 years ago
|
||
(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•17 years ago
|
||
Any reason you're not using an ifdef in browser.xml instead of a run-time check?
Comment 79•17 years ago
|
||
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-
Comment 80•17 years ago
|
||
(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.
Assignee | ||
Comment 81•17 years ago
|
||
(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•17 years ago
|
||
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•17 years ago
|
Attachment #258603 -
Attachment is obsolete: true
Comment 83•17 years ago
|
||
Firefox has the same problem with custom themes. In the default one, however, the autoscroll cursor is normal.
Assignee | ||
Comment 84•17 years ago
|
||
(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•17 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.
Assignee | ||
Comment 86•17 years ago
|
||
(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•17 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.
Comment 88•17 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.
FYI, you might be refering to this: https://bugzilla.mozilla.org/show_bug.cgi?id=305746
Comment 89•17 years ago
|
||
(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
Comment 90•17 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•17 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.
Assignee | ||
Comment 92•17 years ago
|
||
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•17 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).
Comment 94•17 years ago
|
||
Before this change, the mouse cursor used to change when hovering over the autoscroll icon. Was it removed on purpose?
Assignee | ||
Comment 95•17 years ago
|
||
(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).
You need to log in
before you can comment on or make changes to this bug.
Description
•