Closed
Bug 382466
Opened 18 years ago
Closed 15 years ago
in toolbar.xml, prevent lots of repeated work in updateChevron() by using a timer
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 3.7a1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
blocking1.9.1 | --- | - |
status1.9.1 | --- | wontfix |
People
(Reporter: moco, Assigned: mak)
References
Details
(Keywords: perf, Whiteboard: [TSnappiness])
Attachments
(2 files, 7 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
beltzner
:
approval1.9.2+
|
Details | Diff | Splinter Review |
in toolbar.xml's itemChanged(), prevent lots of repeated work in updateChevron() by using a timer
spun off from bug #379591, asaf wrote:
> Hrm, it might worth checking whether boxObject.width of the DOM node has
> changed (on a timeout) and only call updateChevron if so.
In addition to loading or reloading livemarks, there might be a lot of calls to updateChevron when adding or deleting lots of top level personal toolbar items from the bookmark manager dialog at once.
again, before we fix this, we should be convinced that we have a real performance problem.
Comment 1•18 years ago
|
||
>
> spun off from bug #379591, asaf wrote:
>
> > Hrm, it might worth checking whether boxObject.width of the DOM node has
> > changed (on a timeout) and only call updateChevron if so.
is there a DOM mutation event we could listen for? that might be the most efficient way to go, if it's possible.
Updated•16 years ago
|
OS: Windows XP → All
Hardware: x86 → All
Updated•16 years ago
|
Whiteboard: [TSnappiness]
Comment 2•15 years ago
|
||
There's the underflow/overflow event, which could be used at least if the items were in a <xul:scrollbox> (which might be a good idea anyway).
Assignee | ||
Comment 3•15 years ago
|
||
i'm experimenting with the scrollbox, i've a working solution with some bug, i'm actually evaluating if we can at the same time reduce work calling updateChevron on a timer and fix bug 447571. Ideally i'd like to completely remove the need to call updateChevron manually, and let the task to dom listeners.
Assignee: nobody → mak77
Assignee | ||
Comment 4•15 years ago
|
||
This should be better than the previous impl, i'm using a stack instead of the vbox with hacked negative margins, the scrollbox allows to handle in a more efficient way overflow/underflow cases, resize handles window resizes.
All work is done on a timer, so that if we have many changes coming in a small timeframe we will delay the update to a single final one.
Finally this fixes a problem with the indicator bar that after a drag&drop was preventing the toolbar to shrink, causing the chevron to go out of scope.
From my tests this also solves bug 447571, the behavior looks similar to the one we had before it regressed, i can eventually provide a tryserver build to users having the issue and get feedback.
Attachment #392488 -
Flags: review?(dao)
Assignee | ||
Comment 5•15 years ago
|
||
i also tried to get rid of the remainig calls to updateChevron, using further listeners, but i had to listen also on other 3 mutation events and filter them. this way update it's called just when it's needed and does not generate a bunch of useless calls to be filtered out.
Comment 6•15 years ago
|
||
You can probably get rid of toolbar-drop-indicator-bar altogether and at the same time avoid the stack. The latest patch in bug 347930 did this for the tab bar.
Assignee | ||
Comment 7•15 years ago
|
||
thanks, i'll look into that.
Comment 8•15 years ago
|
||
(In reply to comment #6)
> You can probably get rid of toolbar-drop-indicator-bar altogether and at the
> same time avoid the stack. The latest patch in bug 347930 did this for the tab
> bar.
I've filed bug 508499 for that, so you can see that patch without all the noise around it.
Assignee | ||
Comment 9•15 years ago
|
||
Comment on attachment 392488 [details] [diff] [review]
patch v1.0
i had already a working solution based on it, there was not much noise. I still have to fix RTL and then it should be ready for review
Attachment #392488 -
Attachment is obsolete: true
Attachment #392488 -
Flags: review?(dao)
Assignee | ||
Comment 10•15 years ago
|
||
i have a problem in RTL, the scrollbox overflows on the wrong side (right), i suspect i'm missing some align, any hint?
Comment 11•15 years ago
|
||
(In reply to comment #10)
> i have a problem in RTL, the scrollbox overflows on the wrong side (right), i
> suspect i'm missing some align, any hint?
Have you tried playing with different alignments? I'd expect the scrollbox to overflow on the left side in RTL mode by default...
Assignee | ||
Comment 12•15 years ago
|
||
yes, i've tried some alignment, could be i'm missing something obvious
Assignee | ||
Comment 13•15 years ago
|
||
this fixes the drop indicator, but scrollbox problem still exists.
Assignee | ||
Comment 14•15 years ago
|
||
+ else if (this.childNodes.length) {
this brace should not be there, fixed locally
replacing the scrollbox with an hbox overflows in the right direction. but i don't get anymore the overlow events clearly.
Comment 15•15 years ago
|
||
You may find this part of tabbrowser's addTab method interesting:
> if (this.tabContainer.mTabstrip._isRTLScrollbox) {
> /* In RTL UI, the tab is visually added to the left side of the
> * tabstrip. This means the tabstip has to be scrolled back in
> * order to make sure the same set of tabs is visible before and
> * after the new tab is added */
>
> this.tabContainer.mTabstrip.scrollByPixels(this.mTabs[0].clientWidth);
> }
I also think this shouldn't be necessary.
Assignee | ||
Comment 16•15 years ago
|
||
So, for some reason i'm starting thinking scrollbox solution is not the best:
Scrollbox binding does not have an implementation, so i would have to use arrowscrollbox, and collapse the arrows.
Due to the above RTL bug on every update i should scroll back the box to position 0.
Since i'm adding 2 new listeners in the constructor could hurt startup by some ns.
I don't need to scroll and will never need, arrowscrollbox is bringing with it a quite complex binding that i'll never use.
the hbox solution on the other side works just as fine, the only drawback is that with overflow event i can directly uncollapse the chevron, and if it is collapsed i avoid doing any update to it. While, with the hbox solution, i have to check if we are overflowed during the update process.
Due to the fact the update now happens on a timer, the overhead should be mitigated for the most part, and largely better than what we actually have.
If we really want to try using a scrollbox, i'd move that to a followup and take current advantages from this patch.
Attachment #392694 -
Attachment is obsolete: true
Attachment #392822 -
Flags: review?(dao)
Comment 17•15 years ago
|
||
(In reply to comment #16)
> So, for some reason i'm starting thinking scrollbox solution is not the best:
> Scrollbox binding does not have an implementation, so i would have to use
> arrowscrollbox, and collapse the arrows.
I don't understand what you mean. What implementation are you referring to?
Assignee | ||
Comment 18•15 years ago
|
||
well, i mean, what you can do is QI its boxObject to nsIScrollBoxObject and use the implemented scrolling methods. arrowscrollbox is fairly more complex and complete. but still other problems do not give me confidence.
I think we should file a followup about converting toolbar to a scrollbox, and make it depend on a bug on the scrollbox RTL issue, there are not many advantages in the end.
Comment 19•15 years ago
|
||
You can use scrollbox.scrollLeft. I don't think you need anything else, but maybe I'm missing something.
Assignee | ||
Comment 20•15 years ago
|
||
i did not try scrollLeft, so eventually i can try that too.
As a further reason i don't like this, it makes the scrollbox jumpy, since i will scroll on the same update timer that updates the chevron.
eventually i can provide both patches, there aren't many changes needed between the two, i had a working patch, just was not seeing the advantage in doing that right now, rather than waiting a proper fix for the scrollbox.
Assignee | ||
Comment 21•15 years ago
|
||
yes, i'll provide both patches so we can better evaluate the differences.
Comment 22•15 years ago
|
||
The rtl fix should probably look something like this:
//XXX bug [yet to be filed]
if (rtl)
scrollBox.scrollLeft = scrollBox.scrollWidth;
Why should that happen in a timer callback?
Assignee | ||
Updated•15 years ago
|
Attachment #392822 -
Attachment is obsolete: true
Attachment #392822 -
Flags: review?(dao)
Assignee | ||
Comment 23•15 years ago
|
||
Comment on attachment 392822 [details] [diff] [review]
patch v2.0
filed bug 508816 about the scrollbox issue.
I'm going with the scrollbox, i've provided some more cleanup of the code. There is still work needed but we plan to do another D&D code cleanup soon, so i won't go far more than this.
Assignee | ||
Comment 24•15 years ago
|
||
Attachment #392956 -
Flags: review?(dao)
Assignee | ||
Comment 25•15 years ago
|
||
// XXX (bug xxxxxx) Scrollbox does not handle correctly RTL mode.
this should be bug #, i was pretty sure to have replaced those xxx, but sounds like i forgot, fixed locally.
Comment 26•15 years ago
|
||
Comment on attachment 392956 [details] [diff] [review]
patch v3.0
>+ <xul:hbox pack="start">
>+ <xul:image class="toolbar-drop-indicator"
>+ mousethrough="always"
>+ collapsed="true"/>
>+ </xul:hbox>
mousethrough="always" can be removed, I think.
> <method name="_rebuild">
> <body><![CDATA[
>- // Clear out references to existing nodes, since we'll be deleting and re-adding.
>+ // Clear out references to existing nodes, since they will be removed
>+ // and re-added.
there's a superfluous space at the start of the second line
> <method name="chevronPopupShowing">
> <parameter name="aEvent"/>
> <body><![CDATA[
>+ // Handle popupshowing only for the chevron popup.
> var popup = aEvent.target;
> if (popup != this._chevron.firstChild)
> return;
make this if (aEvent.target != aEvent.currentTarget) ...?
>+ <method name="handleEvent">
>+ <parameter name="aEvent"/>
> <body><![CDATA[
>+ switch (aEvent.type) {
>+ case "resize":
>+ // Ignore events that aren't on the document or the window
>+ // (html document, tooltips, etc)
>+ // Do not ignore content window resizes, because they may
>+ // be the result of the toolbar being shown/hidden
>+ if (aEvent.target != document && aEvent.target != window &&
>+ aEvent.target != content)
>+ return;
It seems that aEvent.target != document will always be true, as the event is dispatched on the window.
Did you test if aEvent.target != content is still needed?
>+ case "overflow":
>+ // Attach the popup binding to the chevron popup if it has not yet
>+ // been initialized.
The comment needs another space before //.
>+ <method name="_updateChevronInternal">
This needs a better name, I think. Maybe _updateChevronTimerCallback.
>+ <body><![CDATA[
>+ var spaceLeft = this._scrollbox.getBoundingClientRect().width;
> for (var i = 0; i < this.childNodes.length; i++) {
> var child = this.childNodes[i];
>+ spaceLeft -= child.getBoundingClientRect().width;
>+ child.style.visibility = spaceLeft < 0 ? "hidden" : "visible";
> }
What if the items have a margin? Shouldn't this rather work with getBoundingClientRect().right for ltr and .left for rtl?
> if (aParentPopup._endMarker != -1) {
>- aParentPopup.insertBefore(element,
>- aParentPopup.childNodes[aParentPopup._endMarker]);
>+ var lastNode = aParentPopup.childNodes[aParentPopup._endMarker];
>+ aParentPopup.insertBefore(element, lastNode);
> }
nit: use let
>+ var nodeIndex = this._getChildIndex(xulNode);
Array.indexOf(this.childNodes, xulNode);?
> if (PlacesUtils.nodeIsFolder(xulNode.node) &&
> !PlacesUtils.nodeIsReadOnly(xulNode.node)) {
>+ var threshold = nodeBCR.width * 0.25;
>+ else {
>+ // Drop after this folder.
>+ var beforeIndex =
>+ nodeIndex == this.childNodes.length - 1 ? -1 : nodeIndex + 1;
> else {
>+ // This is a non-folder node or a read-only folder.
>+ // Drop before it with regards to RTL mode.
>+ var threshold = nodeBCR.width * 0.5;
>+ else {
>+ // Drop after this bookmark.
>+ var beforeIndex =
nit: use let
>- // The _clearOverFolder() function will close the menu for _overFolder.node.
>- // So null it out if we don't want to close it.
>+ // The _clearOverFolder() function will close the menu for
>+ // _overFolder.node. So null it out if we don't want to close it.
What's that change about?
>+ switch(direction) {
>+ case "ltr":
...
>+ break;
>+ case "rtl":
...
>+ break;
> }
use if/else instead
>+/* Bookmarks toolbar */
> .toolbar-drop-indicator {
> position: relative;
>+ z-index: 1;
>+ list-style-image: url(chrome://browser/skin/places/toolbarDropMarker.png);
> }
position: relative; and z-index: 1; belong in browser/base/content/browser.css.
>--- a/browser/themes/pinstripe/browser/browser.css
>+++ b/browser/themes/pinstripe/browser/browser.css
It looks like this file hasn't been updated properly.
Updated•15 years ago
|
Attachment #392956 -
Flags: review?(dao)
Assignee | ||
Comment 27•15 years ago
|
||
(In reply to comment #26)
> >+ if (aEvent.target != document && aEvent.target != window &&
> >+ aEvent.target != content)
> >+ return;
>
> It seems that aEvent.target != document will always be true, as the event is
> dispatched on the window.
>
> Did you test if aEvent.target != content is still needed?
i'm testing it, hopefully some check can go
> >- // The _clearOverFolder() function will close the menu for _overFolder.node.
> >- // So null it out if we don't want to close it.
> >+ // The _clearOverFolder() function will close the menu for
> >+ // _overFolder.node. So null it out if we don't want to close it.
>
> What's that change about?
just 80 char limit
> >+/* Bookmarks toolbar */
> > .toolbar-drop-indicator {
> > position: relative;
> >+ z-index: 1;
> >+ list-style-image: url(chrome://browser/skin/places/toolbarDropMarker.png);
> > }
>
> position: relative; and z-index: 1; belong in browser/base/content/browser.css.
would then not be better browser/components/places/content/places.css? This is only used in Places toolbar.xml
Comment 28•15 years ago
|
||
(In reply to comment #27)
> just 80 char limit
I think you can spare that change. The comment is harder to read after your reformatting, fwiw.
> would then not be better browser/components/places/content/places.css? This is
> only used in Places toolbar.xml
Yep, sounds right.
Assignee | ||
Comment 29•15 years ago
|
||
(In reply to comment #26)
> (From update of attachment 392956 [details] [diff] [review])
> >+ <xul:hbox pack="start">
> >+ <xul:image class="toolbar-drop-indicator"
> >+ mousethrough="always"
> >+ collapsed="true"/>
> >+ </xul:hbox>
>
> mousethrough="always" can be removed, I think.
removing it causes indicator flickering when the mouse is dragging over the indicator. This could be an issue of our code that draws it, but i won't investigate it here. i'm leaving it in for now.
Fixed a bug in handling overflow/underflow events, the toolbar should not handle those events for scrollboxes in descendant menupopups.
Fixed remaining comments.
Attachment #392956 -
Attachment is obsolete: true
Attachment #393868 -
Flags: review?(dao)
Comment 30•15 years ago
|
||
Comment on attachment 393868 [details] [diff] [review]
patch 3.1
>+ <xul:toolbarbutton type="menu"
>+ class="chevron"
>+ mousethrough="never"
>+ collapsed="true"
>+ tooltiptext="&bookmarksToolbarChevron.tooltip;"
>+ onpopupshowing="onChevronPopupShowing(event);">
>+ <xul:menupopup anonid="chevronPopup"
>+ xbl:inherits="tooltip"
The onpopupshowing attribute should be on the menupopup element (and onChevronPopupShowing should use aEvent.currentTarget instead of aEvent.currentTarget.lastChild).
>+ <field name="_isRTL">
>+ // To test UI interactions in RTL mode you can set intl.uidirection.en
>+ // string pref to rtl. Add-ons like ForceRTL won't work correctly since
>+ // this field could be initialized before them.
Note that _isRTL won't actually be updated when it was initialized before intl.uidirection.en changed.
In any case, intl.uidirection.* needs to be documented in a central place rather than adding such a comment to every piece of code that deals with rtl.
>+ <method name="_updateChevronPopupNodesVisibility">
> <body><![CDATA[
>+ var chevronPopup = this._chevron.lastChild;
nit: wrongly indented line
>+ for (var i = 0; i < chevronPopup.childNodes.length; i++) {
>+ chevronPopup.childNodes[i].hidden =
>+ this.childNodes[i].style.visibility != "hidden";
>+ }
nit: "i" makes more sense as "let", as you don't want to use it outside of the loop.
>+ <method name="handleEvent">
>+ <parameter name="aEvent"/>
>+ <body><![CDATA[
>+ // Both overflow/underflow and resize events should not be handled
>+ // for descendant nodes.
Did you really get overflow/underflow events for nested nodes?
>+ // This handles case when [...]
There's something wrong with that sentence's grammar.
>+ <method name="_updateChevronTimerCallback">
>+ <body><![CDATA[
>+ var sboxBCR = this._scrollbox.getBoundingClientRect();
Maybe just call this scrollRect?
>+ // Once a child overflows, all the next ones will.
>+ if (!childOverflowed) {
>+ var childBCR = child.getBoundingClientRect();
And similarly, childRect?
Also, a nice candidate for "let" instead of "var"...
>+ childOverflowed = (this._isRTL && childBCR.left < sboxBCR.left) ||
>+ (!this._isRTL && childBCR.right > sboxBCR.right);
I would recommend:
childOverflowed = this._isRTL ? (A)
: (B);
instead of:
childOverflowed = (this._isRTL && A) ||
(!this._isRTL && B);
> <method name="_getDropPoint">
>+ if (xulNode.node) {
>+ var nodeBCR = xulNode.getBoundingClientRect();
let nodeRect ...?
>+ if ((this._isRTL && aEvent.clientX > nodeBCR.right - threshold) ||
>+ (!this._isRTL && aEvent.clientX < nodeBCR.left + threshold)) {
>+ else if ((this._isRTL && aEvent.clientX > nodeBCR.left + threshold) ||
>+ (!this._isRTL && aEvent.clientX < nodeBCR.right - threshold)) {
>+ if ((this._isRTL && aEvent.clientX > nodeBCR.left + threshold) ||
>+ (!this._isRTL && aEvent.clientX < nodeBCR.left + threshold)) {
if (this._isRTL ? (A)
: (B)) {
instead of:
if ((this._isRTL && A) ||
(!this._isRTL && B)) {
>+ else if (aTimer == this._ibTimer) {
>+
>+ this._dropIndicator.collapsed = true;
> this._ibTimer = null;
> }
That line break looks uninteded.
Assignee | ||
Comment 31•15 years ago
|
||
(In reply to comment #30)
> (From update of attachment 393868 [details] [diff] [review])
> >+ <xul:toolbarbutton type="menu"
> >+ class="chevron"
> >+ mousethrough="never"
> >+ collapsed="true"
> >+ tooltiptext="&bookmarksToolbarChevron.tooltip;"
> >+ onpopupshowing="onChevronPopupShowing(event);">
> >+ <xul:menupopup anonid="chevronPopup"
> >+ xbl:inherits="tooltip"
>
> The onpopupshowing attribute should be on the menupopup element (and
> onChevronPopupShowing should use aEvent.currentTarget instead of
> aEvent.currentTarget.lastChild).
the popup is filled up in onpopupshowing, moving the onpopupshowing on the popup would execute it before the popup is populated, making this call useless. the function should then be enqueued, making the code far more complex than needed.
> >+ <field name="_isRTL">
> >+ // To test UI interactions in RTL mode you can set intl.uidirection.en
> >+ // string pref to rtl. Add-ons like ForceRTL won't work correctly since
> >+ // this field could be initialized before them.
>
> Note that _isRTL won't actually be updated when it was initialized before
> intl.uidirection.en changed.
sure, browser needs to be restarted between changes.
> In any case, intl.uidirection.* needs to be documented in a central place
> rather than adding such a comment to every piece of code that deals with rtl.
i agree, i put that in as a side note since people could try to use forceRTL method to work on the toolbar UI. i can remove the comment fwiw.
> >+ <method name="handleEvent">
> >+ <parameter name="aEvent"/>
> >+ <body><![CDATA[
> >+ // Both overflow/underflow and resize events should not be handled
> >+ // for descendant nodes.
>
> Did you really get overflow/underflow events for nested nodes?
places popups can contain scrollboxes when the number of children is really big, and yes, i was getting overflow events from them.
> >+ childOverflowed = (this._isRTL && childBCR.left < sboxBCR.left) ||
> >+ (!this._isRTL && childBCR.right > sboxBCR.right);
>
> I would recommend:
>
> childOverflowed = this._isRTL ? (A)
> : (B);
>
> instead of:
>
> childOverflowed = (this._isRTL && A) ||
> (!this._isRTL && B);
i tried that before since i prefer the ? : form usually, but makes the thing harder to read, being a condition that returns a condition (it's a = b ? c > d : e < f). Someone does not appreciate this kind of things.
> >+ if ((this._isRTL && aEvent.clientX > nodeBCR.right - threshold) ||
> >+ (!this._isRTL && aEvent.clientX < nodeBCR.left + threshold)) {
>
> >+ else if ((this._isRTL && aEvent.clientX > nodeBCR.left + threshold) ||
> >+ (!this._isRTL && aEvent.clientX < nodeBCR.right - threshold)) {
>
> >+ if ((this._isRTL && aEvent.clientX > nodeBCR.left + threshold) ||
> >+ (!this._isRTL && aEvent.clientX < nodeBCR.left + threshold)) {
>
> if (this._isRTL ? (A)
> : (B)) {
>
> instead of:
>
> if ((this._isRTL && A) ||
> (!this._isRTL && B)) {
as before, i think that will make this unreadable, being these long conditions
Comment 32•15 years ago
|
||
(In reply to comment #31)
> the popup is filled up in onpopupshowing, moving the onpopupshowing on the
> popup would execute it before the popup is populated, making this call useless.
Really? The popup's popupshowing handler is capturing, yours isn't, so I think that should work.
> i agree, i put that in as a side note since people could try to use forceRTL
> method to work on the toolbar UI. i can remove the comment fwiw.
forceRTL could in fact be fixed to update the _isRTL field, so I don't think that comment makes a lot of sense anyway.
> i tried that before since i prefer the ? : form usually, but makes the thing
> harder to read, being a condition that returns a condition (it's a = b ? c > d
> : e < f). Someone does not appreciate this kind of things.
Well, I made that recommendation because I found the chained boolean expressions unnecessarily long-winded and thus harder to read. The ternary expression better represents what's going on: the rtl condition picks the expression that you're actually interested in.
Assignee | ||
Comment 33•15 years ago
|
||
(In reply to comment #32)
> (In reply to comment #31)
> > the popup is filled up in onpopupshowing, moving the onpopupshowing on the
> > popup would execute it before the popup is populated, making this call useless.
>
> Really? The popup's popupshowing handler is capturing, yours isn't, so I think
> that should work.
yes, i thought the same initially, but does not work. the winning popupshowing (the one that fills up the popup) is menu.xml handler. it is also in the capturing phase, but the sequence i receive the notifications (moving popupshowing to the popup) is: toolbar.xml::popupshowing, toolbar.xml::chevronpopupshowing, menu.xml::popupshowing.
Assignee | ||
Comment 34•15 years ago
|
||
fixed all comments, apart popupshowing.
I could maybe add a popupshowing bubbling handler to the binding and use that instead of onpopupshowing, but won't change much.
i changed aEvent.currentTarget.lastChild to this._chevron.lastChild, it's more self-explained
Attachment #393868 -
Attachment is obsolete: true
Attachment #394001 -
Flags: review?(dao)
Attachment #393868 -
Flags: review?(dao)
Comment 35•15 years ago
|
||
(In reply to comment #33)
> > Really? The popup's popupshowing handler is capturing, yours isn't, so I think
> > that should work.
>
> yes, i thought the same initially, but does not work. the winning popupshowing
> (the one that fills up the popup) is menu.xml handler. it is also in the
> capturing phase, but the sequence i receive the notifications (moving
> popupshowing to the popup) is: toolbar.xml::popupshowing,
> toolbar.xml::chevronpopupshowing, menu.xml::popupshowing.
Odd, I tested onpopupshowing="Cu.reportError(this._built)" on the menupopup and it seems to be executed after menu.xml's popupshowing handler.
toolbar.xml's popupshowing handler doesn't matter here, right?
Assignee | ||
Comment 36•15 years ago
|
||
(In reply to comment #35)
> Odd, I tested onpopupshowing="Cu.reportError(this._built)" on the menupopup and
> it seems to be executed after menu.xml's popupshowing handler.
> toolbar.xml's popupshowing handler doesn't matter here, right?
no, toolbar xml popupshowing handles autoopening and d&d issues with Linux, it's the inside menu binding that manages the popups contents.
i just added some console dump to the threee handlers after moving the onpopupshowing handler to the popup, and what i got was the above order. So i'm not sure why you see the reportError executed after :\
Comment 37•15 years ago
|
||
Comment on attachment 394001 [details] [diff] [review]
patch 3.2
The calculation of the indicator's position appears to be incorrect when there are other items on the toolbar.
>+ <xul:hbox pack="start">
>+ <xul:image class="toolbar-drop-indicator"
>+ mousethrough="always"
>+ collapsed="true"/>
>+ </xul:hbox>
This stretches the indicator vertically. You need to use align="center".
Why did you add pack="start"?
> else {
> // Dragging over a normal toolbarbutton,
> // show indicator bar and move it to the appropriate drop point.
>+ var ind = this._dropIndicator;
>+ var halfInd = ind.getBoundingClientRect().width / 2;
>+ var direction = document.defaultView
>+ .getComputedStyle(this.parentNode, "")
>+ .direction;
>+ var translateX;
These should also be let.
Attachment #394001 -
Flags: review?(dao) → review-
Assignee | ||
Comment 38•15 years ago
|
||
(In reply to comment #37)
> (From update of attachment 394001 [details] [diff] [review])
> The calculation of the indicator's position appears to be incorrect when there
> are other items on the toolbar.
oh, nice catch.
I tried to add a toolbarbutton on the left and one on the right and the results are now correct. I also tried to move the home button on the toolbar, and everything looks fine.
> >+ <xul:hbox pack="start">
> >+ <xul:image class="toolbar-drop-indicator"
> >+ mousethrough="always"
> >+ collapsed="true"/>
> >+ </xul:hbox>
>
> This stretches the indicator vertically. You need to use align="center".
> Why did you add pack="start"?
i merely copied that from your first patch for the tabbar and forget to check it.
Btw, i don't know if the effect was due to the stretching, but the indicator was looking better to me, clearer and crisper. I suppose that's because the dotted blue line was appearing like a continue line, and that was looking really cleaner and more similar to the tabbar drop indicator style. Maybe we could investigating replacing the image, i don't know why originally that was done as a dotted line. What do you think?
Attachment #394001 -
Attachment is obsolete: true
Attachment #394248 -
Flags: review?(dao)
Comment 39•15 years ago
|
||
(In reply to comment #38)
> > This stretches the indicator vertically. You need to use align="center".
> > Why did you add pack="start"?
>
> i merely copied that from your first patch for the tabbar and forget to check
> it.
I think my patches always used align="start", not pack.
> Maybe we
> could investigating replacing the image, i don't know why originally that was
> done as a dotted line. What do you think?
The dotted line looks fine to me. I'm not sure if a solid one would look better. Maybe ask Stephen or Alex about it...
Comment 40•15 years ago
|
||
Comment on attachment 394248 [details] [diff] [review]
patch v3.3
There's overflow: hidden; for hbox[type="places"] in places.css. That needs to be removed in order to prevent the indicator from being cut off.
> > mousethrough="always" can be removed, I think.
>
> removing it causes indicator flickering when the mouse is dragging over the
> indicator.
I can't reproduce this. Please make sure it's really needed.
>+ <xul:scrollbox orient="horizontal"
>+ class="bookmarks-toolbar-items"
>+ style="min-width: 1px"
What's the min-width achieving? Can this be removed?
>+ this._scrollbox.addEventListener("overflow", this, false);
>+ this._scrollbox.addEventListener("underflow", this, false);
>+ window.addEventListener("resize", this, false);
Also remove these event listeners in the destructor?
>+ <field name="_chevron">
>+ document.getAnonymousElementByAttribute(this, "class", "chevron")
>+ </field>
Rather than doing this._chevron.lastChild over and over again, I think a _chevronPopup field should be added.
>+ <field name="_isRTL">
>+ document.defaultView.getComputedStyle(this.parentNode, "")
>+ .direction == "rtl"
>+ </field>
.direction is strangely indented.
>+ <method name="onChevronPopupShowing">
That name should probably start with an underscore.
>+ if (this._updateChevronTimer) {
>+ this._updateChevronTimer.cancel();
>+ this._updateChevronTimer = null;
> }
>+ this._updateChevronTimer = this._setTimer(100);
this._updateChevronTimer = null; is redundant.
>+ // If the chevron popup is open, maintain it in sync.
s/maintain/keep/
>+ // This function returns informations about where to drop when
s/informations/information/
>+ let beforeIndex =
>+ nodeIndex == this.childNodes.length - 1 ? -1 : nodeIndex + 1;
This could use some brackets, e.g. (nodeIndex == this.childNodes.length - 1)
Attachment #394248 -
Flags: review?(dao) → review+
Assignee | ||
Comment 41•15 years ago
|
||
(In reply to comment #40)
> > > mousethrough="always" can be removed, I think.
> >
> > removing it causes indicator flickering when the mouse is dragging over the
> > indicator.
>
> I can't reproduce this. Please make sure it's really needed.
as soon as i remove it, i see the flickering, so it's really needed
fixed all other comments.
Attachment #394248 -
Attachment is obsolete: true
Assignee | ||
Comment 42•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Target Milestone: --- → Firefox 3.7
Comment 43•15 years ago
|
||
Marco, is this going to land on the 1.9.2 branch?
Assignee | ||
Comment 44•15 years ago
|
||
(In reply to comment #43)
> Marco, is this going to land on the 1.9.2 branch?
after some baking, if no problems are reported, i think would make sense
Assignee | ||
Updated•15 years ago
|
Target Milestone: Firefox 3.7 → Firefox 3.7a1
Assignee | ||
Comment 45•15 years ago
|
||
Rollup patch including bug 508816
This patch greatly reduces work needed to update the bookmarks toolbar, produces cleaner code, and solves 2 common-issues often reported by users.
Attachment #395030 -
Flags: approval1.9.2?
Assignee | ||
Comment 46•15 years ago
|
||
Updated•15 years ago
|
Flags: blocking-firefox3.6?
Comment 47•15 years ago
|
||
bug 447571 (fixed by this patch) can look like dataloss and is definitely buggy. If this patch is low-risk, can it make 1.9.1 as well?
If not, can we make sure there's at least a relnote?
blocking1.9.1: --- → ?
status1.9.1:
--- → ?
Comment 48•15 years ago
|
||
patches this large are generally not "low-risk". Maybe ask again after it's landed in 1.9.2 and gotten some beta testing.
blocking1.9.1: ? → -
Comment 49•15 years ago
|
||
... and by the time we get beta testing, 1.9.2 will be close to shipping and we likely will have run out of time to ship this in a 1.9.1.x release before 1.9.2 (a minor update, remember) ships. That makes this wontfix on the 1.9.1 branch.
Updated•15 years ago
|
Attachment #395030 -
Flags: approval1.9.2? → approval1.9.2+
Comment 50•15 years ago
|
||
Comment on attachment 395030 [details] [diff] [review]
rollup patch for 1.9.2
a192=beltzner
Updated•15 years ago
|
Flags: blocking-firefox3.6? → blocking-firefox3.6-
Assignee | ||
Comment 51•15 years ago
|
||
status1.9.2:
--- → beta1-fixed
Comment 52•15 years ago
|
||
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".
In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body contains places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.
Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.
Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•