77738 - [RFE] need stop button for sidebar

Status: VERIFIED FIXED

(SeaMonkey :: Sidebar, enhancement, P2)

Description

[sujay]

this is a regression.

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

Comment 1

[Blake Ross]

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?

Comment 2

[Matthew T (active 1999-2002)]

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.)

Comment 3

[Viswanath Ramachandran]

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

Comment 4

[selmer (gone)]

m.9.2 seems like the right call...

Comment 5

[Viswanath Ramachandran]

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

Comment 6

[matt]

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.

Comment 7

[Viswanath Ramachandran]

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

Comment 8

[Angela Butler]

Adding mcarlson & myself to the cc: list.

Comment 9

[Viswanath Ramachandran]

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. 

Comment 10

[Samir Gehani]

Taking.

Comment 11

[Samir Gehani]

-> mozilla0.9.8

Comment 12

[Samir Gehani]

Attachment: Resurrecting stop button for tabs.

Comment 13

[Samir Gehani]

morse, please r.
alecf, please sr.

Comment 14

[Stephen P. Morse]

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

r=morse

Comment 15

[Jaime Rodriguez, Jr.]

what are the chances this is gonna make 098?

Comment 16

[Alec Flett]

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

Comment 17

[Samir Gehani]

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.)

Comment 18

[Alec Flett]

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 :)

Comment 19

[Samir Gehani]

Attachment: Incorporated sanity check of button "type".

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 20

[Stephen P. Morse]

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

r=morse

Comment 21

[Alec Flett]

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.

Comment 22

[Samir Gehani]

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?

Comment 23

[Alec Flett]

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

Comment 24

[Samir Gehani]

Sidebar triage team: nsbeta1+

Comment 25

[Samir Gehani]

Attachment: Patch rev 3

Comment 26

[Samir Gehani]

morse, please r.  
alecf, please sr.  

Thanks.  

Comment 27

[Stephen P. Morse]

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

r=morse

Comment 28

[Alec Flett]

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

yay.. much better :)

Comment 29

[Samir Gehani]

Checked in.

Comment 30

[sujay]

verified in 2/20 build