this is a regression. there needs to be a stop button while a sidebar tab is loading

No, sidebar panels should show focus with a border, and then the buttons, menuitems etc. should apply to it. Matthew, do you know there that bug is?

It's bug 42758, but I don't think that's what the reporter was referring to -- I think the problem is that the Stop button isn't enabled while a sidebar panel is loading. Right? (The Stop button should apply to both the sidebar *and* all frames, at once, not just to whichever sidebar/frame is focused.)

Kevin: we'd like to move this to mozilla0.9.2.

m.9.2 seems like the right call...

Kevin - I'm moving this to 0.9.2.. We have regressions in sidebar that are more critical to fix right now. thanks, Vishy

quick summary:I need to figure out why the stop button is not working right now. Most likely it is a css problem. time:2 days reason:I went back and forth with kevin and german about this. I believe by fixing the other problems the user will not need control a stop button in the pane. I also don't think that we want to put ui in a panel rather then having the ui at the top of the sidebar. It seems kind of strange that I would want to stop my bookmarks window from loading or my history window from loading. It is equally strange having these tabs stop loading. But since they both want this I will support them on this UI.

late in the game, but adding German to be sure that we have a UI spec for this.

Adding mcarlson & myself to the cc: list.

sidebar+pdt triage: the problem with this bug is that it is a nice to have, but it is risky to do it at the very end since we would have to test extensively what happens when people hit stop (i.e. that other regressions were not introduced). unfortunate that this was in the PRD as a top item, but we're not getting it this time. We're also not sure that we've thought through the UI spec for this entirely including reload etc of sidebar tabs. Moving to mozilla1.0.

Taking.

-> mozilla0.9.8

morse, please r. alecf, please sr.

Comment on attachment 62600 [details] [diff] [review] Resurrecting stop button for tabs. r=morse

what are the chances this is gonna make 098?

is there a reason not to use getElementById? It will be faster, not to mention it will keep your code from depending on the structure of the sidebar's DOM

Yes, this code is in a template. (Although I do agree that it is a (separate) bug that this template needs an overhaul to eliminate redundancy at which time we would have only one content area that is applicable to the currently active panel. Changes of this nature will also be affected by the sidebar redesign that UE is planning which may allow multiple panels to be open simultaneously.)

ack! then we need an even better way to avoid depending on the structure, because lots of things could change that might affect this - the datasource, the template generation code itself, etc. How about something like content.getElementsByTagName(), or.. hmm. There's got to be a better way :) two thoughts: 1) can the template generate ID's? Because if so you could have an attribute like id="stop-?panel" or something? I don't know enought about the sidebar datasource to know what each arc has, but maybe you can generate unique, well-known ids... 2) getElementsByTagName() at least guarantees a pre-order traversal, so that should allow at least some flexibility when it comes to the DOM structure below the current content node - i.e. it can be rearranged without breaking code at the very least, you need to somehow verify that you've hit the right node, either by giving it a magic attribute (i.e. buttonname="stop") and verifying the existence of that attribute. That sanity check may save you or someone else lots of debugging/headaches down the road :)

It appears that one can't create attribute values with prefixes like the desired "stop-?panel" example here. DOM inspector reveals that the id attribute was not even created on the stop button elements. I have added the sanity check by taking on a type="stop" attribute and verifying it dynamically in the JS. morse, please r. alecf, please sr. Thanks.

Comment on attachment 64570 [details] [diff] [review] Incorporated sanity check of button "type". r=morse

Comment on attachment 64570 [details] [diff] [review] Incorporated sanity check of button "type". >+ >+function toggleLoadarea(content) >+{ >+ // toggle between "loading" and "load stopped" in the UI >+ var stopButton = content.firstChild.firstChild.childNodes.item(3); >+ var reloadButton = content.firstChild.firstChild.childNodes.item(4); >+ var loadingImage = content.firstChild.firstChild.firstChild; >+ var loadingText = loadingImage.nextSibling; >+ var loadStoppedText = loadingText.nextSibling; ok, I'm sorry to get back to you so late on this, but given the proliferation of all this .firstchild.childNodes(N), you really need to break these out into abstract functions like function getSidebarWidgetBox(content) { return content.firstChild.firstChild; } function getStopButton(node) {return node.childNodes.item(3);} and finally var widgetBox = getSidebarWidgetBox(content); var stopButton = getStopButton(widgetBox); The thing to rememer is, even DOM calls result in XPConnect calls into content objects, and in the code you've presented, content.firstChild (which calls nsIDOMElement::GetFirstChild()) is being called multiple times here on the same nodes. You could also bring the sanity checking into each getFooButton() function.. or you could even just check the toplevel box in getSidebarWidgetBox() to make sure you got the right toplevel box, since the format of the template is fixed once you're inside that box. The rest of this is ok.

Alec, Instead of making these function calls, how about I just assign them to vars as follows (minimizing the number of firstChild resolutions needed and avoiding extra function calls): function toggleLoadarea(content) { // toggle between "loading" and "load stopped" in the UI var widgetBox = content.firstChild.firstChild; var widgetBoxKids = widgetBox.childNodes; var stopButton = widgetBoxKids.item(3); var reloadButton = widgetBoxKids.item(4); var loadingImage = widgetBox.firstChild; var loadingText = loadingImage.nextSibling; var loadStoppedText = loadingText.nextSibling; If we need to reuse the widgetBox from another caller in the future we can wrap it in a function at that time. What do you think?

that's fine.. realize there's very very little overhead to adding the extra function :)

Sidebar triage team: nsbeta1+

morse, please r. alecf, please sr. Thanks.

Comment on attachment 68169 [details] [diff] [review] Patch rev 3 r=morse

Comment on attachment 68169 [details] [diff] [review] Patch rev 3 yay.. much better :)

Checked in.

verified in 2/20 build