Closed Bug 182082 Opened 22 years ago Closed 17 years ago

Help system should support more browsers

Categories

(Bugzilla :: User Interface, enhancement, P3)

Product:
Bugzilla
Component:
User Interface
Version:
2.17
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 3.2

People

(Reporter: gerv, Assigned: wicked)

References

(
URL
)

Details

Attachments

(1 file, 5 obsolete files)

Help system browser support, V3.2
16.43 KB, patch
wicked
: review+
Details | Diff | Splinter Review 
The new help system arrived in bug 114179. Currently, it only supports
Mozilla-based browsers (Mozilla/5). To add support for another browser you need to:

- change template/en/default/global/help.html.tmpl to produce suitable HTML for
the popups. There is currently an implementation based on <div>s - this should
be suitable for most modern browsers, but the browser sniffing needs to be
relaxed on it.

- change template/en/default/global/help-header.html.tmpl to produce suitable JS
to animate the HTML created above. There is currently an implementation using
document.getElementById() and other standards; again, this should be suitable
for modern browsers.

- relax the sniffing in template/en/default/search/search.html.tmpl . In fact,
we should be setting a template var "help_enabled" when we create the <div>s or
the JS, so we don't have to duplicate our sniffing in every file which has a
"help" link. 

Gerv
This is probably my problem.

Gerv
Assignee: myk → gerv
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.18
I should say that I have no plans here to add support for anything other than IE
5+ and Konqueror. Trying Netscape 4 support would be a horrible hack, and I
don't have access to copies of Opera. Galeon and friends are already included
because we look for Mozilla/5.

Gerv
IE appears to have the same problem as Mozilla does with select scrollbars, but
worse - it draws form controls over everything :-( Apparently, there's no way to
fix this, so it looks like this method of providing Help just won't fly. So, the
question arises: what do we do next? Remove the Help system entirely? Or just
leave it for Mozilla-based browsers only, where it works fairly well?

Boy, that sucks.

Gerv
What about iemac, or safari?

What ie version did you test?
Finding a Mac to test on is a bit more complicated... and we certainly don't
have Safari installed yet. If someone else wants to test my test installation
(URL above) with those browsers, feel free.

Gerv
Reassigning back to Myk.  That stuff about Gerv taking over the User Interface
component turned out to be short-lived.  Please pardon our confusion, and I'm
very sorry about the spam.
Assignee: gerv → myk
Assignee: myk → gerv
Gerv, 

I want help add the help into IE.....
Found the below bit of code online that seems to do the jobs of the mouse over
popup....works in IE but not in Firefox
How would I go about adding this in ?

<html>
<head>
	<title>Untitled</title>

<SCRIPT LANGUAGE=javascript>
<!--
  function mouseOver() {
    var notes = document.body.all["TheAnswer"], left, top;
    if ((event.clientY + 20 + notes.offsetHeight) >  document.body.offsetHeight) {
      left = event.x + 40;
      top = document.body.offsetHeight - notes.offsetHeight +
document.body.scrollTop - 10;
    } else {
      left = event.x;
      top = event.y + 20;   
    }
    notes.style.posTop = top;
    notes.style.posLeft = left;
    notes.style.visibility = "visible"; 
  }
  
  function mouseOut() {
    var notes = document.body.all["TheAnswer"];  
    notes.style.visibility = "hidden";
  }
//-->
</SCRIPT>
</head>

<body>

<div id=TheAnswer style='position:absolute; width:400; visibility:hidden;
background-color:yellow; BORDER-BOTTOM: 

navy solid thin; BORDER-LEFT: navy solid thin; BORDER-RIGHT: navy solid thin;
BORDER-TOP:  navy solid thin; font-

size:12px'> <br><p align=center><b><font face="verdana" size=4>Recht von Unrecht
unterscheiden können</font></

b><P></div>

<br>
<br>

<font face="verdana" size=4 onmouseover="mouseOver()"
onmouseout="mouseOut()";>Know right from wrong.</font>


</body>
</html>
(In reply to comment #3)
> IE appears to have the same problem as Mozilla does with select scrollbars, but
> worse - it draws form controls over everything :-( 

Is this what you mean by for example the help box being shown behind the
selection boxes ?

Oh yeah....simple one line fix to show this - change the line in
/template/en/default/global/help-header.html.tmpl from <script
type="application/x-javascript"> to <script language="javascript">
Mat:

The code to support mouseovers in IE would need to go in
template/en/default/global/help-header.html.tmpl . The code you quote has a few
good ideas, but it's IE-specific, and I'd rather have one W3C-compliant piece of
code working for both browsers.

The current code shouldn't be too far off working. Change the aforementioned
file to enable the help link for everyone, and then mess around with it in IE.

Gerv
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Have a nose at www.xbox-linux.org and you will see that they have some mouseover
code thats pretty nifty. I tested it in Firefox and IE with no probs
The current code actually does work in IE with very little tweaking; the problem
is that the scrollbars are native, and so show through. :-(

Gerv
At present this leaves NO help system for various browsers including IE...

The old https://bugzilla.mozilla.org/queryhelp.cgi  seems to redirect to
https://bugzilla.mozilla.org/query.cgi?format=advanced&help=1

... and the latter (in IE) gives a script error on line 39 saying "Object 
Expected".


Manually inspecting the source, there seems to be a
  <body bgcolor="#ffffff" onload="doOnSelectProduct(0); initHelp();"
with no initHelp() function actually defined anywhere...

If it's not going to define the initHelp function perhaps it could remove the 
call too? ... and possibly in the same circumstances, replace the text
"For help, mouse over the page elements."
with something like
"Bugzilla thinks your browser sucks so we're not giving you any help."  ;-)
This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to nobody@bugzilla.org or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.
Target Milestone: Bugzilla 2.20 → ---
Target Milestone: --- → Bugzilla 2.20
Blocks: 286802
No longer blocks: 286802
*** Bug 286802 has been marked as a duplicate of this bug. ***
Severity: normal → major
Flags: blocking2.20+
Attached patch Help system browser support, V1 (obsolete) — — Splinter Review 
This patch fixes browser support and few problems mentioned in this bug report.


 1) Enabled help for IE 5.5 and 6. Needed to use an iframe control behind the
div to hide any form controls. Also, worked around div position values in IE 6.
Thanks, MS.
 2) Enabled help for Opera 7. This seems to work with same code as Mozilla.
Older versions might work too but I can't test them.
 3) Centralized browser detection to global/help-header.html.tmpl template. It
will set help_enabled if current browser is supported and help_iehack if
current browser is IE.
 4) Fixed div positioning on Firefox by adding "px" after the calculated
numbers. Without this change FF didn't move the div so it appeared at a fixed
position after the query form.
 5) An empty initHelp() Javascript function is now also defined if browser is
not supported.
 6) A friendly error message is displayed in query.cgi if help=1 is used but
current browser is not supported.

I tested this on IE 3, 4, 5.01 (NT SP2), 5.5 (NT SP2), 6 (XP SP2) and Opera
7.11 and Firefox 1.0.2. Note that all IE versions were run on my XP. BTW, IE 3
couldn't log in succesfully..

What's missing? Somebody should test this patch with an older Opera browsers
because current Mozilla code might work on them as is. This same goes for other
browsers. If so, help system could be enabled for them. I think writing support
for Netscape 4 could be possible but I also think it's waste of time. That
version should just die..
Assignee: gerv → wicked
Status: NEW → ASSIGNED
Attachment #180595 - Flags: review?(gerv)
Comment on attachment 180595 [details] [diff] [review]
Help system browser support, V1

Not only does IE 6 count differently, it counts differently in both strict mode
and non-strict mode.

Also, using a browser-detect is not the right way to handle this. The best way
is to check if the actual elements like pixelTop exist, and use those if they
do, but fallback to the other ones otherwise. Create a function called
"get_position" or something.

I actually have a library of appropriate functions that I've taken from
http://www.quirksmode.org/ that does these sorts of things pretty well. I'll
attach it to give you an idea of what we should be heading toward, for a
general-purpose cross-browser JavaScript library for Bugzilla.
Attachment #180595 - Flags: review?(gerv) → review-
Attached file Sample of Cross-Browser JS library (obsolete) — 
You can use any code in this for Bugzilla, if you want to. :-)
(In reply to comment #12)
> "Bugzilla thinks your browser sucks so we're not giving you any help."  ;-)

Ummmm.... that was intended to be tongue-in-cheek...

(attachment 180595 [details] [diff] [review])
+    [% IF NOT help_enabled AND help %]
+      Sorry, help system is not supported on your browser.
+    [% END %]


If by whatever method it has determined that the help system will not work on 
the browser, I would rather see something like:

+    [% IF NOT help_enabled AND help %]
+      Sorry, help system is not supported on your browser, however you can
+      <a href='query.cgi?help=1&amp;format=advanced&amp;help_enabled=1'>
+      try to get help anyway</a> (please let us know if it works), or check
+      <a href='http://www.bugzilla.org/docs/tip/html/query.html'>
+      Searching for Bugs</a> in
+      <a href='http://www.bugzilla.org/docs/tip/html/'>The Bugzilla Guide</a>
+    [% END %]

Admittedly the "Searching for Bugs" page in the bugzilla guide could do with a 
bit more explanation for stuff other than boolean charts, but that's another 
bug.

Also, *something* should be emitted in the [ NOT help_enabled AND NOT help ] 
case. Otherwise users of unsupported browsers may assume that there is no help 
available at all, and may go away rather than realising they could have had 
help by using a different browser.
"If it's not a regression from 2.18 and it's not a critical problem with
something that's already landed, let's push it off." - Dave
Flags: blocking2.20+
Flags: blocking2.20-
With the kind of re-writing that we're doing now for this bug, I think it's much
more a 2.22 thing.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
I put my alpha V2 patch live at landfill (see URL). Help is now always enabled
and uses an iframe control to retrieve popup contents from the server. It still
has few prominent bugs like blank popup at startup, popup mispositioned for list
control and popup has an awkward size.

I'm not sure if I'm heading right direction with this. Any comments would be
welcome.

Somehow I think, if help is always enabled, it would be better to add a question
mark icon for form controls that have help enabled. When user clicks it a help
popup is opened. Popup content is retrieved from the server and could probably
even be a real window popup instead of a div/iframe control.

myk, UI is your thing, any opionion?
URL: http://landfill.bugzilla.org/bz182082...
QA Contact: mattyt-bugzilla → default-qa
I find the frequent loads this patch requires to be pretty jarring, and the help loads much slower with this patch than it currently does (so slowly, in fact, that it may slow down users browsing through fields, even on a fast connection).  Also, hitting the "back" button goes back through the help iframes before it returns to the previous page loaded in the browser, a significant regression.

Is there no simpler way (even one with drawbacks) to get the current help system working in IE?
I originally suggested the iframes, but perhaps there would be an easier way; I believe wicked had an easier way originally.

Particularly noting the problems that myk mentioned, iframes are probably not the way to go.
(In reply to comment #22)
> Is there no simpler way (even one with drawbacks) to get the current help
> system working in IE?
(In reply to comment #23)
> I originally suggested the iframes, but perhaps there would be an easier way; I
> believe wicked had an easier way originally.

Yeah, my original patch that mkanat r-'d had a solution to use iframe control to blank out controls that are under the div on IE. This might also help on Firefox that sometimes bleed through the scrollbar controls. That iframe control wasn't used to load/show anything but just moved under the original div control to hide stuff.

I think I'll update my first patch to be more browser agnostic using the new JS utility library and enable help when the browser supports needed functions and controls. This will remove direct browser detection code.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24

        Attachment #180595 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Help system browser support, V3 (obsolete)
        — 
          — Splinter Review
      

    


  
Here's my next attempt to add cross-browser support to our help system. This patch will:
 1) Add js/util.js that contains the cross-browser functions suggested by mkanat. I had to fix some of them to actually work, though. :)
 2) Move style and JavaScript from global/help-header.html.tmpl and global/help.html.tmpl templates to skins/standard/help.css and pages/help.js.tmpl files (enables code reuse from separate files).
 3) Improve the help JS by
    1) using object detection instead of browser detection 
    2) using only one div control, which is populated with help text loaded to a JS associative array
    3) using an iframe control to mask any controls under the popup so they won't bleed through in IE or any other browser

I tested this with both IE6 and Firefox 1.5.x and it seems to work. Code should detect automatically if JS is disabled or that JS implementation doesn't support certain key objects.

        Attachment #214506 -
        Attachment is obsolete: true

        Attachment #214981 -
        Flags: review?

    
    

    
  
Putting help.js.tmpl where you have put it is really a misuse of the "pages" mechanism. "Pages" was designed to allow admins to add Bugzilla-branded simple HTML pages to their installations; we also use it for a few standalone utilities like the bug linkifier. But help.js should be a JS file, so it's cacheable and consistent with our other JS files.

I like the idea of util.js, although I believe that all browsers that we currently support have getElementById, so the getObj() function is not required.

The point of help.html.tmpl, which you have removed, was to avoid the use of document.write, which mixes content into behaviour. What is the advantage of moving away from that?

Gerv

    
  

        Attachment #214981 -
        Flags: review? → review?(mkanat)

    
    

    
  
Comment on attachment 214981 [details] [diff] [review]
Help system browser support, V3

I guess this is r- then.

        Attachment #214981 -
        Flags: review?(mkanat) → review-

    
    

    
  
This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2

    
    

    
  


  
I moved help JS to js/help.js and resurrected help.html.tmpl template but I'm still using getObj() function just in case.

        Attachment #214981 -
        Attachment is obsolete: true

        Attachment #258694 -
        Flags: review?(mkanat)

        Attachment #258694 -
        Flags: review?(LpSolit)

    
    

    
  
Comment on attachment 258694 [details] [diff] [review]
Help system browser support, V3.1

Compared to me, myk is a much better reviewer when talking about JS.

        Attachment #258694 -
        Flags: review?(LpSolit) → review?(myk)

    
    

    
  
Comment on attachment 258694 [details] [diff] [review]
Help system browser support, V3.1

>+ * Contributor(s): Max Kanat-Alexander <mkanat@bugzilla.org>

Nit: shouldn't you be listed here and in the other files as well?


>+        //obj.style = document.getElementById(name).style;
...
>+        //obj.style = document.all[name].style;

Is there a reason to leave these commented out statements in the file?


>+// Get an object in a cross-browser compatible way
>+function bz_getObj(name) {

Isn't this more about getting an element or a node than an object?


>+    // Older IE
>+    } else if (document.all) {

Nit: else on a new line.


>+ * Params:  phase = Initialization phase, can be
>+ *                   1 for generating help controls during page construction
>+ *                   2 for enabling help popups for all form elements after
>+ *                         the page has finished loading
>+ * Returns: A boolean indicating if help was enabled or not.

Combining these two tasks into a single function doesn't make sense to me given that there's virtually no sharing of code between them.  Seems like this should be two functions, f.e. generateHelp and enableHelp.


>+ */
>+function initHelp(phase) {
>+    // Validate phase
>+    if (phase < 1 && phase > 2) return false;

Nit: block on a new line here and elsewhere:

    if (phase < 1 && phase > 2)
        return false;


>+        // Create help controls (a div to hold help text and an iframe
>+        // to mask any and all controls under the popup)
>+        document.write('<div id="help_div" style="display: none;"></div>');
>+        document.write('<iframe src="about:blank" id="help_iframe"');
>+        document.write('        frameborder="0" scrolling="no"');
>+        document.write('        style="display: none;"></iframe>');

Nit: id as the first attribute in the list for both the div and the iframe.

Nit: shouldn't the style rule here (display: none) be in the CSS?

Nit: The IDs here would be better as helpIframe and helpDiv, which is consistent with the names of the JS variables that refer to them (or at least as help-div and help-iframe, as dash is the common separator in IDs and class names that don't get reflected into JS).


>+        for (var i = 0; i < document.forms.length; i++) {
>+            for (var j = 0; j < document.forms[i].elements.length; j++) {
>+                /* MS decided to add fieldsets to the elements array; and
>+                 * Mozilla decided to copy this brokenness. Grr.
>+                 */
>+                if (document.forms[i].elements[j].tagName != 'FIELDSET') {
>+                    document.forms[i].elements[j].onmouseover = showHelp;
>+                }
>+            }
>+        }

Nit: omit braces for one-line conditional and loop blocks here and elsewhere, i.e.:

        for (var i = 0; i < document.forms.length; i++)
            for (var j = 0; j < document.forms[i].elements.length; j++)
                /* MS decided to add fieldsets to the elements array; and
                 * Mozilla decided to copy this brokenness. Grr.
                 */
                if (document.forms[i].elements[j].tagName != 'FIELDSET')
                    document.forms[i].elements[j].onmouseover = showHelp;


>+/* Add help text for a control
>+ * Params:  id   = id of a control for which to add help
>+ *          help = help text to use, can contain HTML markup
>+ */
>+function addHelpText(id, help) {
>+    helpTexts[id] = help;
>+}

This function seems unnecessary, given that it replaces a one-line assignment with a one-line function call that does the same assignment, and the assignment is to a global variable, so there's no encapsulation.


>+function showHelp() {
>+    if (helpIframe && helpDiv && helpTexts[this.name]) {
>+        // Get the position and size of the form element in the document
>+        var elemY = bz_findPosY(this);
          ...

Nit: this would be simpler as:

function showHelp() {
    if (!helpIframe || !helpDiv || !helpTexts[this.name])
        return;

    // Get the position and size of the form element in the document
    var elemY = bz_findPosY(this);
    ...

Otherwise, this looks good. r=myk

        Attachment #258694 -
        Flags: review?(myk) → review+

    
    

    
  


  
Added JavaDoc style descriptions, used correct MPL boilerplates and addressed review comments. Carrying over r+.

I did end up removing bz_getObj() as there are already lots of places where we use document.getElementById() and, as pointed out by gerv in comment #27, getElementById() is supported on all browsers we support.

Also, I retained style attribute on written out helpDiv control because without it browsers won't recalculate width and height of the control correctly when innerHtml attribute is changed.

        Attachment #180625 -
        Attachment is obsolete: true

        Attachment #258694 -
        Attachment is obsolete: true

        Attachment #261605 -
        Flags: review+

        Attachment #258694 -
        Flags: review?(mkanat)

    
  
Flags: approval?

    
    

    
  
(In reply to comment #32)
> Nit: shouldn't you be listed here and in the other files as well?

I agree with myk. Why not adding your name to the list of contributors?
Flags: approval? → approval+

    
    

    
  
RCS file: /cvsroot/mozilla/webtools/bugzilla/js/help.js,v
done
Checking in js/help.js;
/cvsroot/mozilla/webtools/bugzilla/js/help.js,v  <--  help.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/js/util.js,v
done
Checking in js/util.js;
/cvsroot/mozilla/webtools/bugzilla/js/util.js,v  <--  util.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/help.css,v
done
Checking in skins/standard/help.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/help.css,v  <--  help.css
initial revision: 1.1
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.99; previous revision: 1.98
done
Checking in template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v  <--  header.html.tmpl
new revision: 1.50; previous revision: 1.49
done
Removing template/en/default/global/help-header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/help-header.html.tmpl,v  <--  help-header.html.tmpl
new revision: delete; previous revision: 1.6
done
Checking in template/en/default/global/help.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/help.html.tmpl,v  <--  help.html.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/search/search-advanced.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search-advanced.html.tmpl,v  <--  search-advanced.html.tmpl
new revision: 1.30; previous revision: 1.29
done

Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED

    
  
Severity: major → enhancement
Keywords: relnote

    
    

    
  
Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.
Keywords: relnote

    
  
Blocks: 450598

    
  
Blocks: 344917

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: