MARQUEE attribute values must be lower-case to work.

RESOLVED FIXED in mozilla1.0.1

Status

SeaMonkey
General
P1
major
RESOLVED FIXED
16 years ago
13 years ago

People

(Reporter: Daniel Clemente, Assigned: Doron Rosenberg (IBM))

Tracking

({testcase})

Trunk
mozilla1.0.1
x86
All
testcase
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2 RTM] [ETA 08/01], URL)

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

16 years ago
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.1b) Gecko/20020727
BuildID:    20020727

Hi.

   The MARQUEE tag has a parameter DIRECTION which can be set to LEFT or RIGHT,
but also to UP or DOWN in order to do a vertical scroll (just like a film's
credits), that is not implentented yet in Mozilla.

   You can check this at my simple webpage
http://www.danielclemente.com/html/index.htm#scroll
   or writing the HTML code: <MARQUEE DIRECTION="UP">this is a test</MARQUEE>
  
   Thanks,
          Daniel Clemente


Reproducible: Always
Steps to Reproduce:
Open an .HTM that contains this code:

<MARQUEE DIRECTION="UP">this is a test</MARQUEE>


Actual Results:  The text is quiet.

Expected Results:  The text should move from bottom to top.

<MARQUEE DIRECTION="UP">this is a test</MARQUEE>
<MARQUEE DIRECTION="DOWN">this is a test</MARQUEE>

(LEFT and RIGHT work ok, but UP and DOWN do not)
<MARQUEE DIRECTION="LEFT">this is a test</MARQUEE>
<MARQUEE DIRECTION="RIGHT">this is a test</MARQUEE>

Comment 1

16 years ago
Created attachment 93019 [details]
scrolling up marquee works

Adding another sample - which seems to work fine:

<marquee behavior=scroll direction=up width=130 height=80 scrollamount=1
scrolldelay=0>
This works pretty well
Are you sure your code was valid?
</marquee>

Comment 2

16 years ago
nothing moves with attached testcase 
win98 2002072508
(Reporter)

Comment 3

16 years ago
Ok, I've got it:

   I'm sure, 
<MARQUEE DIRECTION="UP">this is a test</MARQUEE>
   doesn't work.

   But this:
<MARQUEE DIRECTION="up">this is a test</MARQUEE>
   works perfectly.

   So, the problem is the case.

Comment 4

16 years ago
From bug 80269: "Will <Marquee> tag be supported?"

> <marquee> is a Microsoft-proprietary tag and is not in any HTML
> standard, thus it is unlikely that Mozilla will support it.

==> VERIF/WONTFIX


Status: UNCONFIRMED → RESOLVED
Last Resolved: 16 years ago
Depends on: 80269
Resolution: --- → WONTFIX

Updated

16 years ago
Whiteboard: verifyme

Comment 5

16 years ago
FYI: A XBL emulation of the MARQUEE tag was checked in yesterday (bug 156979)
So.. umm.. it's kinda supported now.

Comment 6

16 years ago
removing bogus dependency
No longer depends on: 80269

Comment 7

16 years ago
Reopening. This is no longer a wontfix, it's a valid bug in a new element.
Status: RESOLVED → UNCONFIRMED
Resolution: WONTFIX → ---
So this bug should be resummarized to "marquee attribute values must be
lower-case to work", and we need to verify that case-sensitivity is a bug (HTML
is case insensitive, so I expect it is).  I'll leave it to Daniel to resummarize.

/be
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 9

16 years ago
   Probably it's a bug, since HTML is case-insensitive. In addition, if LEFT and
RIGHT values work regardless to the case, UP and DOWN should do the same.

   Let me put this few examples of what works and don't work (build 2002072704)

<MARQUEE DIRECTION="left">this works</MARQUEE>
<MARQUEE DIRECTION="right">this works</MARQUEE>
<MARQUEE DIRECTION="up">this works</MARQUEE>
<MARQUEE DIRECTION="down">this works</MARQUEE>

<MARQUEE DIRECTION="LEFT">this works</MARQUEE>
<MARQUEE DIRECTION="RIGHT">this works</MARQUEE>
<MARQUEE DIRECTION="UP">this DOESN'T work</MARQUEE>          <-----  FIX THIS ----->
<MARQUEE DIRECTION="DOWN">this DOESN'T work</MARQUEE>    <-----  FIX THIS ----->
Summary: Vertical marquee doesn't work → MARQUEE attribute values must be lower-case to work.

Comment 10

16 years ago
In response to comment 5: would it make sense to go back and reopen all
<marquee> related bugs in the database, since we do support it now? Otherwise,
we'll have more confusion duping some bugs to old WONTFIXes and some other ones
to this.

Hmm. I have 2002072608, and it doesn't support it yet. I'm just one day behind
the events, and I'm not current enough already! Talk about bugreports from 2
month old builds. =^.^=

Updated

16 years ago
Whiteboard: verifyme

Comment 11

16 years ago
Attention: I spotted another bug with <MARQUEE DIRECTION="LEFT">, which moves to
the right instead. See attachment.

Comment 12

16 years ago
Created attachment 93045 [details]
testcase

Updated

16 years ago
Keywords: testcase

Comment 13

16 years ago
Ok, so this is related to Bug 156979.  

Comment 14

16 years ago
Will submit a patch in a few.

Comment 15

16 years ago
Created attachment 93046 [details] [diff] [review]
patch

Please don't pull my tail too hard, it's my first try. =^.^=

Updated

16 years ago
Keywords: patch, review

Comment 16

16 years ago
Comment on attachment 93046 [details] [diff] [review]
patch

Index: xbl-marquee.xml
===================================================================
RCS file:
/cvsroot/mozilla/layout/html/document/src/xbl-marquee/resources/content/xbl-mar
quee.xml,v
retrieving revision 1.2
diff -u -r1.2 xbl-marquee.xml
--- xbl-marquee.xml	Sat Jul 26 21:07:03 2002	1.2
+++ xbl-marquee.xml	Sat Jul 27 21:06:00 2002
@@ -135,7 +135,7 @@
	   {
	     this.scrollStarted = 1; //we only want this to run once

-	     switch (this.direction)
+	     switch (this.direction.toLowerCase())
	     {
	       case "up":
		 this.dirsign = 1;
@@ -151,18 +151,18 @@
		 this.stopAt  = 0;
	       break;

-	       case "left":
-		 this.dirsign = 1;
-		 this.startAt = this.innerDiv.offsetLeft -
this.outerDiv.offsetWidth;
-		 this.stopAt  = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth + this.startAt;
-	       break;
-
	       case "right":
-	       default:
		 this.dirsign = -1;
		 this.stopAt  = this.innerDiv.offsetLeft -
this.outerDiv.offsetWidth;
		 this.startAt = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth + this.stopAt;	      
	       break;
+
+	       case "left":
+	       default:
+		 this.dirsign = 1;
+		 this.startAt = this.innerDiv.offsetLeft -
this.outerDiv.offsetWidth;
+		 this.stopAt  = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth + this.startAt;
+	       break;
	     }
	     this.newPosition = this.startAt;
	   } //end if
@@ -185,7 +185,7 @@
	     }
	   }

-	   switch(this.direction)
+	   switch(this.direction.toLowerCase())
	   {
	     case "up":
	     case "down":
@@ -223,7 +223,7 @@
	 <![CDATA[
	   var height;

-	   if ((this.direction == "up") || (this.direction == "down")) {
+	   if ((this.direction.toLowerCase() == "up") ||
(this.direction.toLowerCase() == "down")) {
	     height = "200px";
	     this.outerDiv.style.height = height;
	   }
@@ -238,7 +238,7 @@

	   this.outerDiv.style.width = this.width;

-	   if ((this.direction == "up") || (this.direction == "down")) {
+	   if ((this.direction.toLowerCase() == "up") ||
(this.direction.toLowerCase() == "down")) {
	     this.innerDiv.style.padding = height + " 0";
	   }

Comment 17

16 years ago
Created attachment 93047 [details] [diff] [review]
patch (proper one)

Eep. My apologies, first one was a mess. Silly, silly me! Try rhis.

Updated

16 years ago
Attachment #93046 - Attachment is obsolete: true
Wouldn't it be simpler to change the getter for the direction property to do
toLowerCase()?  (You'd still need to do the left/default combination, but not
the other changes.)

Comment 19

16 years ago
Most definitely, but I'm afraid to mess something up. ;)
OK, I'll try and do some serious revamping now.

Comment 20

16 years ago
Could anybody please explain me, is there any specific reason in using this:

      <method name="start">
          if (this.scrollStarted == 0)
          {
            this.scrollStarted = 1; //we only want this to run once
            ...code...
          }
 
instead of putting  ...code...  into init() method?

Assignee: Matti → wesha

Comment 21

16 years ago
Created attachment 93086 [details] [diff] [review]
Updated patch

* Added toLowerCase() to prop retrievers
* Moved a piece of code that was intended to execute only once to init()
* Optimized switch()
Attachment #93047 - Attachment is obsolete: true
(Assignee)

Comment 22

16 years ago
going to steal this, I have some other issues that need to be cleaned up.
OS: Linux → All
(Assignee)

Comment 23

16 years ago
note that the reason the code was in start is that in theory, you can stop the
marquee and change its direction. I just never implemented it fully, which is
the bug I want to fix.
(Assignee)

Comment 24

16 years ago
Created attachment 93110 [details] [diff] [review]
cleaning up marquee xbl - adding dynamic direction changing, bgcolor and lowercasing
Attachment #93086 - Attachment is obsolete: true
(Assignee)

Comment 25

16 years ago
stealing bug
Assignee: wesha → doron
QA Contact: asa → nobody

Comment 26

16 years ago
Doron, I doubt JS engine has builtin optimizer, so please also remove any
"case"s in "switch" statement that are grouped with "default".

Comment 27

16 years ago
> cleaning up marquee xbl - adding dynamic direction changing, bgcolor
> and lowercasing

Should we update the summary accordingly?

Are any of these problems critical to the Chinese top sites?

Don't we want to have this in RTM, if so, please nominate...
(Assignee)

Updated

16 years ago
Keywords: adt1.0.1
(Reporter)

Comment 29

16 years ago
Created attachment 93192 [details]
alternate marquee bouncing incorrectly
(Reporter)

Comment 30

16 years ago
   Ok, please tell me if I -the reporter- must do anything (it's my first bug).

   I'd like to explain another possible 'bug' of the MARQUEE tag: when we set
BEHAVIOR parameter to ALTERNATE the text should bounce, but shouldn't hide
itself at the borders.

   Check the attachment.
(Assignee)

Comment 31

16 years ago
I am fully aware of that difference, and don't think its that important for
1.0.1.  Lets try to support it as minimally as possible.
And let's keep this bug focused on the case sensitivity problem, please file new
bugs for feature requests.
(Assignee)

Comment 33

16 years ago
anyone want to review this? We'll only get qa probably once its reviewed.
(Assignee)

Comment 34

16 years ago
Created attachment 93217 [details] [diff] [review]
branch patch - uper case working, bgcolor, cleaned up a not needed case:

Comment 35

16 years ago
this looks completely straight forward
r=bstell@ix.netcom.com

Updated

16 years ago
Attachment #93217 - Flags: review+

Updated

16 years ago
Attachment #93110 - Attachment is obsolete: true
Comment on attachment 93217 [details] [diff] [review]
branch patch - uper case working, bgcolor, cleaned up a not needed case:

+	  // change the scroll position
+	   if ((this.direction == "up") || (this.direction == "down"))
...

Minor style nit, fix the indentation of the comment.

sr=jst
Attachment #93217 - Flags: superreview+

Comment 37

16 years ago
In the start(),

              case "left":
                this.dirsign = 1;
                this.startAt = this.innerDiv.offsetLeft - this.outerDiv.offsetWidth;
                this.stopAt  = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth +
this.startAt;
              break;

              case "right":
              default:
                this.dirsign = -1;
                this.stopAt  = this.innerDiv.offsetLeft - this.outerDiv.offsetWidth;
                this.startAt = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth +
this.stopAt;              
              break;

1) default should be "left", not "right".
2) If "case" group contains "default" case, everything but "default" label is
senseless.

Therefore,

              case "right":
                this.dirsign = -1;
                this.stopAt  = this.innerDiv.offsetLeft - this.outerDiv.offsetWidth;
                this.startAt = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth +
this.stopAt;              
              break;

              default:
                this.dirsign = 1;
                this.startAt = this.innerDiv.offsetLeft - this.outerDiv.offsetWidth;
                this.stopAt  = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth +
this.startAt;
              break;



Attention, please DO NOT get "setting default to left in marquee.direction to
"left"" and  this "default"!!! The first one happens if marquee.direction is
empty, while the second happens when it's not empty but isn't one of the keywords.

Comment 38

16 years ago
I am unclear how this is related to making the marquee code case insensitive. 
This sounds like a separate issue. Would you please open a separate bug?

Comment 39

16 years ago
It's more a matter of code optimizaton than a real bug. Besides, only a couple
trivial changes needed to fix it, so why starting the process from the very
beginning when it's possible to add a couple lines to current patch and resolve
it once and forever?

Comment 40

16 years ago
In general the idea of getting multiple things in at once is good but time is
very tight right now and getting an approval is harder the larger the patch is.

Updated

16 years ago
Blocks: 143047
Severity: normal → major
Keywords: mozilla1.0.1, mozilla1.2, nsbeta1+
Priority: -- → P1
Whiteboard: [adt2 RTM] [ETA 08/01]
Target Milestone: --- → mozilla1.0.1

Updated

16 years ago
Keywords: approval

Comment 41

16 years ago
Comment on attachment 93217 [details] [diff] [review]
branch patch - uper case working, bgcolor, cleaned up a not needed case:

a=chofmann for 1.0.1 on this update to the patch for the branch.
Attachment #93217 - Flags: approval+

Comment 42

16 years ago
checked into the branch

Comment 43

16 years ago
posthumus adt1.0.1+. 

brian: pls do not check in to the 1.0 branch, without ADT approval. ADT approval
is expressed with a "adt1.0.1+" in Keywords, and a comment from the ADT that the
patch is approved for checking into the branch. thanks!
Keywords: adt1.0.1 → adt1.0.1+

Comment 44

16 years ago
Sorry, I thought a=chofmann was approval.

Comment 45

16 years ago
brian: a=chofmann is drivers' aproval for the branch. netscape employees
checking into the branch are also required to have ADT approval before checking
into the 1.0 branch. additionally, once you have checked something into the 1.0
branch, you should add the keyword "fixed1.0.1", so that QA is notified to
verify the fix. thanks!

ji: xianlan, can you pls verify this as fixed on the branch, then replace
"fixed1.0.1" with "verified1.0.1"

doron: will you or brian be checking this into the trunk?
Keywords: mozilla1.0.1 → fixed1.0.1
QA Contact: nobody → ji

Comment 46

16 years ago
jaime: it's quite ironic that you've called brian on this point given that the
original marquee for 1.0.1 landed a=jaime w/o any driver approval, especially
not for 1_0 which is supposed to get features *after* trunk.

Comment 47

16 years ago
I'll be glad to check it into the trunk once I know it has approval.

Comment 48

16 years ago
ji - can you verify this bug fix in branch?  When verified, pls replace
fixed1.0.1 keyword with verified1.0.1.  Thanks.

Comment 49

16 years ago
2002082304/WinXP - testcase still doesnt work.

Comment 50

16 years ago
> ji - can you verify this bug fix in branch?  When verified, pls replace
> fixed1.0.1 keyword with verified1.0.1.  Thanks.

ji is on vacation until 8/27.
Carine can you help?  or Andreas is there someone else that can?
(Assignee)

Comment 51

16 years ago
verified on 1.0.1 branch
Status: NEW → ASSIGNED
Keywords: fixed1.0.1 → verified1.0.1
(Reporter)

Comment 52

16 years ago
   Should someone check-in the patch into the trunk, or first the bug must be
marked as RESOLVED ?
   With Linux 2002100604, the bug is present yet.

Comment 53

15 years ago
This bug is still present in Mozilla 1.4.

I also note that some HTML examples sites (such as
http://www.htmlcodetutorial.com/_MARQUEE_DIRECTION.html) don't use any lower
case for their HTML examples.

Comment 54

14 years ago
I concur with the analysis.

In addition, the "alternate" behavior NEVER works (it should bounce from side to
side; instead it slides across like a normal marquee).

<marquee width=100% behavior=alternate>test</marquee>
That code bounces on the right side, but slides out on the left before returning
to the right. Kinda weird, if you ask me.

Mozilla 1.5, Win32, build 20030925.
(Assignee)

Comment 55

14 years ago
lower-case is now fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago14 years ago
Resolution: --- → FIXED

Comment 56

14 years ago
Fixed by what? 

->WORKSFORME
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

14 years ago
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → WORKSFORME

Comment 57

14 years ago
Doh!  My apologies.  The patch in this very bug.  (Slapping myself on the face
and resetting.)
Resolution: WORKSFORME → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.